refactor: revamp pagination UI view logic #10567
Conversation
| const beforeLastPage = numPages - 1; | ||
| const startPage = leftBound > 2 ? leftBound : 2; | ||
| const endPage = rightBound < beforeLastPage ? rightBound : beforeLastPage; | ||
| if (numPages <= TOTAL_PAGE_BLOCKS) { |
There was a problem hiding this comment.
The diff looks bigger than it is because I moved the "easy" case to the top to reduce some of the function's nesting. Might be worth looking at this with diffs turned off?
There was a problem hiding this comment.
love me a good early return, v nice
| if (!disabled && showDisabledMessage) { | ||
| setShowDisabledMessage(false); | ||
| } |
There was a problem hiding this comment.
Yeah, I remember seeing an article about it called something like "the ugliest pattern in React" or something.
It is better than using useEffect from a performance standpoint, and I guess also "philosophy" reasons (the React team is getting more bullish about how useEffect should be used strictly to sync with external systems, and if the state already exists inside React, you should do things like this)
There was a problem hiding this comment.
and yet we also have the wonderful useSyncExternalStore hook :lolsob:
There was a problem hiding this comment.
I mean, I get that the hook was mainly designed to help state management tools hook into React, but yeah. React's gonna React
| disabled: boolean; | ||
| disabledMessage: ReactNode; | ||
| onClick: () => void; | ||
| "aria-label": string; |
There was a problem hiding this comment.
Rather than special case this one prop, we should probably just extend HTMLAttributes
There was a problem hiding this comment.
I can extend it, but the thing is that even if I extend HTMLAttributes or componentPropsWithoutRef, I'll still need to treat aria-label as a special case to enforce that it's required
| onClick, | ||
| disabled, | ||
| disabledMessage, | ||
| "aria-label": ariaLabel, |
There was a problem hiding this comment.
...and instead of destructuring, just catch extra props with ...attrs...
| aria-label={ariaLabel} | ||
| aria-disabled={disabled} |
There was a problem hiding this comment.
...and then spread props onto the target with {...attrs}
| if (!isFirstPage) { | ||
| onChange(page - 1); | ||
| if (!onFirstPage) { | ||
| onPageChange(currentPage - 1); |
There was a problem hiding this comment.
could we rename this to onChangePage? just feels a lot more natural to me
There was a problem hiding this comment.
Yeah, I generally go with the onNoun naming convention. You can have a page change, but you can't have a change page, if that makes sense?
There was a problem hiding this comment.
I usually think of it as onAction 🤔 "a change" is kind of a weird "noun". I won't block on it, just my 2¢
There was a problem hiding this comment.
Just double-checked MDN, and all the more specialized change-based follow the onXChange convention
https://developer.mozilla.org/en-US/docs/Web/Events
I am open to changing prop names for clarity, but my thinking is that it's best to mirror the base HTML standards as closely as possible (same reason why I agree with extending the base HTMLAttributes when possible)
| @@ -1,67 +1,74 @@ | |||
| /** | |||
| * Generates a ranged array with an option to step over values. | |||
| * Shamelessly stolen from: | |||
| * https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/from#sequence_generator_range | |||
| * {@link https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/from#sequence_generator_range} | |||
| const beforeLastPage = numPages - 1; | ||
| const startPage = leftBound > 2 ? leftBound : 2; | ||
| const endPage = rightBound < beforeLastPage ? rightBound : beforeLastPage; | ||
| if (numPages <= TOTAL_PAGE_BLOCKS) { |
There was a problem hiding this comment.
love me a good early return, v nice
Connected to #10452 and #10549
Part 1 (of 2?) for the changes I'm hoping to make for our pagination logic. All the updates here deal with view rendering.
Changes made
PaginationWidgetBasecomponent and itsutilsfileNumberedPageButtonandPlaceholderPageButton(there were just too many concerns for one component to address without getting messy/tangled)