- Notifications
You must be signed in to change notification settings - Fork929
refactor: revamp pagination UI view logic#10567
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
love me a good early return, v nice
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.
couple small thoughts, but overall looks nice!
| if (!disabled && showDisabledMessage) { | ||
| setShowDisabledMessage(false); | ||
| } |
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 still don't like it 💀
ParkreinerNov 8, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
Yeah, I remember seeing an article about it called something like "the ugliest pattern in React" or something.
It is better than usinguseEffect from a performance standpoint, and I guess also "philosophy" reasons (the React team is getting more bullish about howuseEffect 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
and yet we also have the wonderfuluseSyncExternalStore hook :lolsob:
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 mean, I get that the hook was mainly designed to help state management tools hook into React, but yeah. React's gonna React
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| disabled: boolean; | ||
| disabledMessage: ReactNode; | ||
| onClick: () => void; | ||
| "aria-label": string; |
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.
Rather than special case this one prop, we should probably just extendHTMLAttributes
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 can extend it, but the thing is that even if I extendHTMLAttributes orcomponentPropsWithoutRef, I'll still need to treataria-label as a special case to enforce that it's required
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.
eh, fair enough.
| onClick, | ||
| disabled, | ||
| disabledMessage, | ||
| "aria-label": ariaLabel, |
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.
...and instead of destructuring, just catch extra props with...attrs...
| aria-label={ariaLabel} | ||
| aria-disabled={disabled} |
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.
...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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
could we rename this toonChangePage? just feels a lot more natural to me
ParkreinerNov 8, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
Yeah, I generally go with theonNoun 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I usually think of it asonAction 🤔 "a change" is kind of a weird "noun". I won't block on it, just my 2¢
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.
Just double-checked MDN, and all the more specialized change-based follow theonXChange 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 baseHTMLAttributes 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 | |||
| *{@linkhttps://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/from#sequence_generator_range} | |||
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.
mr. fancy-js-doc-knower :^)
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
love me a good early return, v nice
Uh oh!
There was an error while loading.Please reload this page.
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)