Skip to content
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

feat(unstable-pagination): add refactored experimental pagination component #5485

Merged
merged 36 commits into from
Jun 2, 2020
Merged

feat(unstable-pagination): add refactored experimental pagination component #5485

merged 36 commits into from
Jun 2, 2020

Conversation

jendowns
Copy link
Contributor

@jendowns jendowns commented Feb 28, 2020

Closes #5129
Closes #4365

This PR has the Unstable_Pagination component, a refactored experimental version of Pagination component that is written with a "building blocks" approach, allowing users to add either an inner PageSelector (traditional select) or PageInput (a number input), or leave out those children entirely. The component also includes icon-only buttons with tooltips.

This component is implemented in our downstream library @carbon/ibm-security, and I copied almost everything from there (changing prefixes and so forth, of course).

Changelog

New

  • add Unstable_Pagination component with demo and styles
  • add PageSelector
  • the pagiantion buttons now have tooltips!! 🎉
  • add PageInput Removed in this PR, to wait for design exploration in Design for pagination number input  #5546

TODO: Tests

I couldn't copy over our tests because we use react testing library for our version of this component -- its' super useful because we have the user event package as well to check tab order etc.

Are you all open to that, or would you rather I rewrite tests in your current structure/style?

Testing / Reviewing

Check out the storybook demo & documentation.

@jendowns jendowns marked this pull request as ready for review February 28, 2020 17:09
@jendowns jendowns requested a review from a team as a code owner February 28, 2020 17:09
@ghost ghost requested review from joshblack and tw15egan February 28, 2020 17:09
@netlify
Copy link

netlify bot commented Feb 28, 2020

Deploy preview for carbon-components-react ready!

Built with commit 7c650ea

https://deploy-preview-5485--carbon-components-react.netlify.com

@netlify
Copy link

netlify bot commented Feb 28, 2020

Deploy preview for carbon-elements ready!

Built with commit 7c650ea

https://deploy-preview-5485--carbon-elements.netlify.com

@netlify
Copy link

netlify bot commented Feb 28, 2020

Deploy preview for carbon-elements ready!

Built with commit 152ba4f

https://deploy-preview-5485--carbon-elements.netlify.app

@netlify
Copy link

netlify bot commented Feb 28, 2020

Deploy preview for carbon-components-react ready!

Built with commit 152ba4f

https://deploy-preview-5485--carbon-components-react.netlify.app

@jendowns
Copy link
Contributor Author

@designertyler This PR is a followup to our conversation in #4365 --

I have an example with a number input added here: https://deploy-preview-5485--carbon-components-react.netlify.com/?path=/story/unstable-pagination--with-a-page-input
How should it look though?
And where should the error state appear? (right now I have hidden the "invalid"/"error" text for the input, and all you see if a red outline if you type in letters)

@joshblack joshblack mentioned this pull request Feb 28, 2020
82 tasks
@joshblack
Copy link
Contributor

@jendowns for the use-case of tables with a large number of pages, what use-case or flow are we considering where they would like to select a specific page in a list of 200,000 pages? 🤔

Copy link
Contributor

@joshblack joshblack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a question above, curious to hear what you think 👀

@jendowns
Copy link
Contributor Author

jendowns commented Feb 28, 2020

@joshblack Here are the use cases I'm seeing...

  • Super large number of pages (10k, 200k, etc) -- don't use PageSelector. Maybe use PageInput if you want and if you are getting data from backend with each page change (othewrise that's... a lot of data lol).
  • 100-ish pages -- could use PageSelector and pass in size={10} to limit options list height (since PageSelector has a spread operator). Or could use PageInput.

I think the key in both cases is to pick the tool that works best, whether it's a select, a number input, or nothing at all. 👍

@jendowns
Copy link
Contributor Author

jendowns commented Feb 28, 2020

OH @joshblack I just realized I kinda missed the point of your question here. 😅

Yeah I don't see why there's a need to jump pages at all, honestly. It seems like you would have to know EXACTLY where you want to go?

But these are just options 🤷‍♀ As said in my previous comment, it's a set of tools that someone can use depending on their situation. If they have 200k pages they should seriously consider NO children for this pagination component and instead provide ways for people to filter/search.

@joshblack
Copy link
Contributor

@jendowns yeah, I think that's 100% along the lines of what I was thinking for this too. It can seem like at that number of pages this feature isn't really meant for your use-case/users.

@jendowns
Copy link
Contributor Author

@joshblack 200k is defintiely an extreme example. What #4365 highlights an issue we have in product right now where there's a reasonable number of pages (80-something, I think) and the page selector is super long in Chrome.

Try it out here in Chrome: https://react.carbondesignsystem.com/?path=/story/pagination--pagination
Set the totalItems to like 500 or something.

@designertyler
Copy link
Contributor

Using the number input in place means we have two sets of buttons with the same function right next to each other.

Screen Shot 2020-03-02 at 10 07 25 AM

There's no existing example of an input in a toolbar like this so we'd have to explore ways to do the error message or limit the ability for the user to cause an error. I'm also not sure if we want to use ui-03 for the field or if we'd want to use something to the mobile number input that uses field-01.

I'll get with the team to see how we want to establish these types of entry fields.

@jendowns
Copy link
Contributor Author

jendowns commented Mar 4, 2020

Thanks @designertyler!

If you prefer, I can remove the PageInput subcomponent (i.e., the inner number input) from this PR for now, while you explore the button & error state questions? (And then when you are finished with the design explorations, I can propose a followup PR, separate from this one.)

That way, I can still get a review on the rest of the code with the hopes of providing good alternatives / paths for people to resolve issues like #5129 & #4365? (That is, with this PR minus the number input, they'd be able to just leave out any type of page selector to avoid causing their browser to crash. OR pass a size attribute to the page selector to limit the size of the options list, if they don't have thousands of pages.)

CC @joshblack @tw15egan

@jendowns
Copy link
Contributor Author

jendowns commented Mar 24, 2020

Hey @tw15egan sorry for the delay! I have fixed the jumpy focus border/outline that you saw when you reviewed. Could you take another look when you get a chance? Thank you! 🎉

@tw15egan
Copy link
Collaborator

@jendowns just another small style issue I noticed. Seems like the spacing between the chevrons are different. Also, the right select does not seem to have any hover styles.

pagination-2

Other than that, everything looks great!

@tw15egan tw15egan requested a review from a team as a code owner April 20, 2020 17:18
@jendowns
Copy link
Contributor Author

@joshblack @tw15egan I updated the hover styles for the "page selector" select element.

I'm not sure about the point about arrow spacing in the select inputs, however. They both have the same svg, the same dimensions, and the same padding:
Screen Shot 2020-05-12 at 1 28 57 PM
Screen Shot 2020-05-12 at 1 29 16 PM

@jendowns
Copy link
Contributor Author

Hey @joshblack @tw15egan @designertyler did you all need anything from me with this PR?

Copy link
Contributor

@designertyler designertyler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Visually it looks good. I think the chevron spacing was a result of there being 1 vs 2 digit numbers next to the chevron.


const { prefix } = settings;

function Unstable_Pagination({
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
function Unstable_Pagination({
function unstable_Pagination({

Will try to make sure this is in our style guide soon! Totally get if the U was meant for usage in React components, I think we would just recommend renaming the import if that's the case:

import { unstable_Pagination as Pagination } from '...';

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joshblack I can update it to unstable_ in all locations (including file names) if that's what you prefer?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes please! The only reason we do this convention is to mirror FB's unstable_ convention, e.g. unstable_SuspenseList. I'll make sure to add a reference to this in our style guide as well, sorry about the mixup

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah no worries @joshblack I probably misremembered the capitalization when I wrote this

One thing I just noticed though, as I started to change to unstable_ -- the linter is not pleased with me & flags hooks usage as a violation of eslint(react-hooks/rules-of-hooks):

Screen Shot 2020-06-01 at 10 13 35 AM

React Hook "useState" is called in function "unstable_Pagination" that is neither a React function component nor a custom React Hook function. eslint(react-hooks/rules-of-hooks)

How do you want to proceed here?

Copy link
Contributor

@joshblack joshblack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely down to bring this in under the unstable namespace 👀 So sorry for the delays @jendowns, thanks so much for being persistent here 🙏

Copy link
Collaborator

@tw15egan tw15egan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good to me! Thanks for taking the time to add this in @jendowns

@tw15egan tw15egan merged commit 40d9deb into carbon-design-system:master Jun 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants