svelte: Improve commit page loading state#62842
Conversation
It was reported that we show the old commit data when navigating to a different commit page. The page was explicitly implemented to show the previously loaded data in case there is an error when fetching more data. But this also affects normal page navigation. In this commit the implementation was changed to return the previously loaded data when an error occurs, which simplifies loading state and data rendering on the page itself.
vovakulikov
left a comment
There was a problem hiding this comment.
Tested manually, not a blocker but with big commits (I usually test on DefinitelyTyped repository) we still show old content as we fetch the new one, so with big commits when it takes senseable time to fetch new commit data it looks like a freeze.
My suggestion here would be to add some inline loader state (maybe against the link which cases transition, or some global loading indicator somewhere at the top of the page.
| list-style: none; | ||
|
|
||
| li { | ||
| li:not(:last-child) { |
There was a problem hiding this comment.
Found not that easy to read selectors with not operator, maybe & + & would be simpler here
vovakulikov
left a comment
There was a problem hiding this comment.
Approve since code LGTM
@vovakulikov do you mean the commit information (title, message, ...)? Showing some kind of navigation indicator is a general problem. We have to find a solution that works across the app. Other places have that issue too (e.g when using fuzzy finder). That requires some design input (cc @taiyab) |
It was reported (Slack thread) that we show the old commit data when navigating to a different commit page.
The page was explicitly implemented to show the previously loaded data in case there is an error when fetching more data. But this also affects normal page navigation.
In this commit the implementation was changed to return the previously loaded data when an error occurs, which simplifies loading state and data rendering on the page itself.
Test plan
Manual testing + new integration test.