-
Notifications
You must be signed in to change notification settings - Fork 18
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ENG-3070 Disable workflows list sorting #1408
ENG-3070 Disable workflows list sorting #1408
Conversation
No ordering: Screen.Recording.2023-06-05.at.2.17.13.PM.mov |
@@ -118,7 +117,7 @@ const NavBar: React.FC<{ | |||
); | |||
} | |||
return ( | |||
<Link | |||
<RouterLink |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did this work? Curious if this helps disabling the refresh when you click on the breadcrumb link
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is how it looks:
Screen.Recording.2023-06-05.at.3.04.01.PM.mov
I wasn't too sure what you meant by disabling the refresh.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I meant did you see lots of API call in your server log when you click on the link? Previously it does as we refetch these APIs, which didn't utilize the caching power of RTK framework
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not a big deal if it doesn't fix. We can file a task and investigate later
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it does. When clicking "workflows" from the workflow detail page of workflow 0, only the details of the workflow 0 was filled in. The rest had to be loaded in from the server.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm okay let's file a separate task then
sortColumn: sortColumns[0], | ||
sortType: SortType.Descending, | ||
}} | ||
// Disabled until ENG-3096 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Add a TODO:
in this comment. This helps us finding and clean up TODO
s if these follow up didn't get in due to other priorities.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for following up and addressing multiple rounds of comments!
Describe your changes and why you are making these changes
Delay the workflows list rendering so that the dates can load in and the rows can order before loading. Will be replaced with a more ideal solution as a future task. Not sure if having a several seconds delay before loading the workflows page is preferred to showing the reordering.
Tested with 100 workflows (modified from Hari's many small workflows example but instead these workflows run once and aren't not scheduled) and S3 as metadata store.
Related issue number (if any)
ENG-3070
ENG-3096
Loom demo (if any)
Jun 5 Update:
No ordering:
Screen.Recording.2023-06-05.at.2.17.13.PM.mov
Before
loading_before.mov
After
loading_after.mov
Checklist before requesting a review
python3 scripts/run_linters.py -h
for usage).run_integration_test
: Runs integration testsskip_integration_test
: Skips integration tests (Should be used when changes are ONLY documentation/UI)