Dashboard: Prevent scroll to top on pagination #2883
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…status to prevent scroll to top on new page load. adding memo() to my stories content and header as they are pure components recieving the same props and LOTS of props. To expand on this later. Limiting search results to first 5 and memoizing, there is no need to pass the whole list of results to the typeahead right now as it is only showing the first few results. Note: come back and enable scrolling.
BrittanyIRL
added
Type: Bug
Codecov Report
@@ Coverage Diff @@
## master #2883 +/- ##
===========================================
- Coverage 81.95% 69.27% -12.69%
===========================================
Files 675 699 +24
Lines 10346 11875 +1529
===========================================
- Hits 8479 8226 -253
- Misses 1867 3649 +1782
|
|
Size Change: +8 B (0%) Total Size: 996 kB
ℹ️ View Unchanged
|
BrittanyIRL
changed the title
swissspidy
reviewed
Jul 1, 2020
| @@ -114,7 +125,13 @@ function Header({ | |||
| handleLayoutSelect={view.toggleStyle} | |||
| currentSort={sort.value} | |||
| pageSortOptions={STORY_SORT_MENU_ITEMS} | |||
| handleSortChange={sort.set} | |||
| handleSortChange={useCallback( | |||
There was a problem hiding this comment.
Nit: I'd have put this into a variable, e.g. const onHandleSortChange = useCallback(...)
There was a problem hiding this comment.
i'll move this in my next pr - thanks!
swissspidy
approved these changes
Jul 1, 2020
|
Karma tests timed out, but will have to wait until #2887 is resolved for this to pass through normally. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Omar identified a bug that we didn't notice with the update to implement scroll to top when you swap from 'all results' to 'draft' or 'publish', we didn't notice it because when that was implemented we were also loading 100 stories at a time so it wasn't as visible - now with our 24 results at a time it's noticeable :D
Bug
Solution
(pardon my glitchy recording)
Paginate without scrolling
Scroll up with switch in filter
Scroll to top on sort update
Relevant Technical Choices
Contentfrom scrolling to top when a new page is loaded and keeps the actions tight.ContentandHeaderin memo since they are pure components that load a lot of props with a lot of data. This will be expanded on later but for now just the default comparison.To-do
User-facing changes
Testing Instructions
Addresses #2884