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(pagination): add props to set size on the "item count" and "page number" select elements #4378

Closed
wants to merge 14 commits into from
Closed

Conversation

jendowns
Copy link
Contributor

@jendowns jendowns commented Oct 17, 2019

Closes #4365

Proposal

Proposal to add a new prop selectSize itemCountSelectSize & pageNumberSelectSize that an app dev could use to set a "size" for the Pagination component inner page number select element, in order to resolve the issue identified in #4365 when there are MANY pages.

The size of the inner select gets updated to the selectSize itemCountSelectSize & pageNumberSelectSize prop numbers (if it's provided) when the select is focussed -- and then it's set back to default / 1 when the select is not focussed.

Background

size is a select attribute that can control the list box size of an open select: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/select

If the control is presented as a scrolling list box (e.g. when multiple is specified), this attribute represents the number of rows in the list that should be visible at one time. Browsers are not required to present a select element as a scrolled list box. The default value is 0.

So I'm thinking it could be useful in this case to use native select functionality with the size attribute to restrict the height of the select list box, in extreme cases like in #4365 where there are many pages. 🤔

Questions

  • What do you think of the selectSize prop name? 😬 Is there a better name? Renamed to itemCountSelectSize & pageNumberSelectSize
  • MDN says the default for Firefox is 0 not 1 🤔 Any objections to setting it to 1 by default though? (It seems to look fine Firefox either way)
  • Do you want the prop on the other select element as well? The one for "items per page"? Added itemCountSelectSize for the other select element.

Changelog

New

  • add selectSize itemCountSelectSize & pageNumberSelectSize props to Pagination
  • add related selectSize itemCountSelectSize & pageNumberSelectSize states
  • add related knob in storybook

Testing / Reviewing

In the netlify deploy for carbon-components-react, open the knobs and change selectSize itemCountSelectSize & pageNumberSelectSize knobs to a number like 5 or 10. Then open the select to change the page. You should see something similar to this:

Screen Shot 2019-10-17 at 4 18 54 PM

@netlify
Copy link

netlify bot commented Oct 17, 2019

Deploy preview for the-carbon-components ready!

Built with commit 4ef0c7f

https://deploy-preview-4378--the-carbon-components.netlify.com

@jendowns jendowns changed the title fix(pagination): add selectSize prop to set size on the page number select feat(pagination): add selectSize prop to set size on the page number select Oct 17, 2019
@netlify
Copy link

netlify bot commented Oct 17, 2019

Deploy preview for carbon-elements ready!

Built with commit 4ef0c7f

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

@netlify
Copy link

netlify bot commented Oct 17, 2019

Deploy preview for carbon-components-react ready!

Built with commit 4ef0c7f

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

@asudoh asudoh requested a review from a team October 17, 2019 23:11
@ghost ghost requested review from designertyler and removed request for a team October 17, 2019 23:11
Copy link
Member

@emyarod emyarod left a comment

Choose a reason for hiding this comment

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

it looks like the default size is 1 according to the spec but the actual implementation in browsers (including Chrome, Firefox, and Edge) defaults to 0

on a side note would this also be a potential issue for the "items per page" select as well?

@jendowns
Copy link
Contributor Author

jendowns commented Oct 18, 2019

@emyarod Yeah in the case identified in #4378 doesn't specifically mention the "items per page" select but it could potentially affect that as well 🤔

How do you want to handle that? New prop? Or maybe this current selectSize prop an object with keys that apply to each select?

@jendowns
Copy link
Contributor Author

jendowns commented Oct 22, 2019

Thanks for the feedback @joshblack & @emyarod!

In my latest commits, I made a couple changes... namely:

  • flattened the styles
  • renamed selectSize prop to pageNumberSelectorSize
  • added itemCountSelectorSize prop that does the same thing for the "items per page" select element.

What do you think about pageNumberSelectorSize and itemCountSelectorSize as prop names? The first part of these props -- pageNumber and itemCount reference the specific class name that identifies the select elements. And then the second part ...SelectorSize refers to "size" that we are changing, and it also gets away from referencing the specific element being changed -- the select element. Instead, it references the function of that element -- that is, it's function as a "selector". 🤔 In the future if you move away from a select element, it's possible that this prop name would still make sense? What do you think?

@jendowns jendowns changed the title feat(pagination): add selectSize prop to set size on the page number select feat(pagination): add props to set size on the "item count" and "page number" select elements Oct 22, 2019
Copy link
Member

@emyarod emyarod left a comment

Choose a reason for hiding this comment

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

I think the prop names are ok as long as they're documented

I want to get design feedback on the modified <select> styles when we change the size though. on Windows there is a flash of the default <select> before the custom menu appears. I think we can avoid custom styles if possible since that's one of the differentiators between <select> and our custom dropdown or overflow menu for example

paginationselect

@jendowns
Copy link
Contributor Author

jendowns commented Oct 23, 2019

Thanks @emyarod! I just pushed an update that sets the selector size onMouseDown so that should cover the situation you saw 🤔

Also could you elaborate on what you mean by this?

I think we can avoid custom styles if possible since that's one of the differentiators between <select> and our custom dropdown or overflow menu for example

The Select is ideal here IMO because it is a form element. The style rules applied in this case are not changing any inner <select> elements (<option>, etc) -- the style changes are superficial & made only so that the the Select doesn't look strange without a background or with the icon showing in the middle of it.

@emyarod
Copy link
Member

emyarod commented Oct 24, 2019

right I am also in favor of keeping a <select> here, but I was under the impression that we could keep the default styles with the size attribute. I'm just curious what design thinks of the <select> styles when size !== 1

@jendowns
Copy link
Contributor Author

@emyarod OH I see what you mean -- I tried to keep the styles to an absolute bare minimum. Have you gotten a chance to look at them when commented out? Here are some screenshots:

Without a background-color on the input + without hiding the icon on :focus --

Screen Shot 2019-10-24 at 9 15 29 AM
Screen Shot 2019-10-24 at 9 15 41 AM

With ALL styles commented out (background-color, hidden icon on :focus, and no position-related rules):

The deal with this in particular is without position: absolute and left/right rules where appropriate, the size attribute isn't honored correctly -- it will only ever be about 2.25 lines tall, no matter what someone passes to the new props.
Screen Shot 2019-10-24 at 9 16 24 AM

@emyarod
Copy link
Member

emyarod commented Oct 24, 2019

@jendowns yeah looks like that's the default behavior when changing the size attribute, so not sure what we can do at the moment. just wanted to make sure it's ok with design

@jendowns
Copy link
Contributor Author

Thanks @emyarod -- you keep mentioning "design", and I was wondering if there is someone in particular we should be tagging here so they see the PR? 😅

Just wanted to point out that the originating issue #4365 is a need from one of our application teams, in a product context. I'm happy to explore other options here but this seems like the least disruptive option available, using a native select attribute? So I would love to get more eyes on this & help out our application team 👍

@designertyler
Copy link
Contributor

The original issue mentioned a text input and that seems like the better solution for navigating through a large list of pages.

User should be able to enter a number to select the page via, for example, a TextInput or a NumberInput component

That would let us avoid the long select menu and long scrolls if the number gets >100. Text input would also be easier for keyboard users who can enter the value rather than keying down to it and mobile users when their OS uses a picker for the select element.

I'd prefer to see a number input solution to close #4365

For this issue of adding selector size knobs I think the visuals are fine if we also allow for aligning to the top or bottom.

image

@jendowns
Copy link
Contributor Author

jendowns commented Oct 24, 2019

Thanks @designertyler --

Changing those select elements to text inputs (or even number inputs) would be a substantial code change. 😬

My assumption is that a change like that would be suited for a major release, and it would probably require some product validation? I would be very curious what our Security Design team thinks of a change like that as well.

EDIT: I just want to be clear, I'm not fundamentally against the idea & of course it's not my decision to make! But it would definitely be out of scope for this PR & I'd want to step back to see some exploration of that before I tackled it myself. 😅


About handling "invalid" entries if using a NumberInput....

Using any kind of input instead of a select raises a couple questions around validation --

  • what happens when someone enters an invalid page number? (is there any feedback?)
  • if there is feedback, where would you show feedback / error messages for both the "items per page" and "page selection" inputs? (and how would that work with product layouts that have a data table above the pagination?)

About select element accessibility...

I also wanted to address this point you made:

Text input would also be easier for keyboard users who can enter the value rather than keying down to it and mobile users when their OS uses a picker for the select element.

You can use the keyboard with a select element! Give it a try -- go to the Pagination, open the page selection Select, and start to type the page you want.

For mobile users, that's a different story. But that goes back to my point above -- the change would be pretty significant and require some validation. 🤔 (To that end, for example: the mobile use case doesn't really apply for the originating issue.) -- Actually I'm not 100% sure about this part.


About alignment for the existing Select solution...

Finally, I had a question about this statement:

For this issue of adding selector size knobs I think the visuals are fine if we also allow for aligning to the top or bottom.

For this,are you referring to something like this? (note: screenshot shows it "fixed" to the top of the pagination component):

Screen Shot 2019-10-24 at 1 53 28 PM

@designertyler
Copy link
Contributor

Yep, number input would need some more review and specs for all those different states. I'll go ahead and create a new issue for that and see if it's a feature we want in the future.

You can use the keyboard with a select element! Give it a try -- go to the Pagination, open the page selection Select, and start to type the page you want.

You're right! 👍

For this,are you referring to something like this? (note: screenshot shows it "fixed" top the top of the pagination component):

Yes, but now I think I take that back. The idea was that it would help when the pagination is at the bottom or top of the screen, but it takes it even further away from the normal select and may have some unintended consequences. What does everyone think?

Another thing I missed is that the normal select dismisses after you select a number, but the custom-sized one doesn't. Can we have that behavior for the custom one?

@jendowns
Copy link
Contributor Author

@designertyler --

The idea was that it would help when the pagination is at the bottom or top of the screen, but it takes it even further away from the normal select and may have some unintended consequences. What does everyone think?

I'm okay with leaving it alone for now. 👍 Something to consider: in the event that someone is coming across that issue, they may need to reduce what they passed itemCountSelectorSize or pageNumberSelectorSize. For example, in your original screenshot above it appears to be a large number, but if it were reduced to something like 5 then the options wouldn't be cut off.


Another thing I missed is that the normal select dismisses after you select a number, but the custom-sized one doesn't. Can we have that behavior for the custom one?

Now for this, I'm not sure 😭

This is an interesting situation because adding a size attribute over 1 makes a select element a "special" type of select. So I think this behavior (where it doesn't dismiss until you tab/click away) is intentional for this type of select?

Maybe you'd have to listen for a selection with the keyboard or mouse and then somehow close the select 🤔 Just thinking out loud. But I'm not a fan of that route, honestly.

@vpicone
Copy link
Contributor

vpicone commented Oct 25, 2019

@jendowns we use size elsewhere to indicate the height/width/type setting on a component. It seems like ideally this wouldn't be hard coded, if the size is different than the number of pages then you get some really weird results.

If a user activates the select, they should just be able to type the page their looking for right? This gets even more complex with dynamically loaded pages/content.

Some issues in the current build, not sure if you're working on these:

after changing the size, the input seems to open from from the middle now

Screen Shot 2019-10-25 at 10 07 59 AM

Overshooting the size gets really wild

Screen Shot 2019-10-25 at 10 08 16 AM

@jendowns
Copy link
Contributor Author

jendowns commented Oct 25, 2019

@vpicone

we use size elsewhere to indicate the height/width/type setting on a component. It seems like ideally this wouldn't be hard coded, if the size is different than the number of pages then you get some really weird results.

When you say "we use size elsewhere" are you referring to just the word "size" in a prop or something else? 🤔

In this case, the "size" attribute on the select element is setting lines visible. I imagine it would always be different than the "number of pages" -- specifically, if you are trying to set the "size" (or "visible lines") to be the same as the "number of pages", then what would be the point? That's just a default select without a custom "size" attribute...


If a user activates the select, they should just be able to type the page their looking for right? This gets even more complex with dynamically loaded pages/content.

I'm not sure what you mean here? This would be an issue with the current implementation as well. It's a select that only knows what options it is given, whether it has a "size" attribute (to show number of options/lines visible) or not. 🤔


after changing the size, the input seems to open from from the middle now

Yes it's absolutely positioned, so that positioning is intentional. @designertyler suggested fixing it so it starts at the top or bottom of the Pagination component, but then decided against that.

But I'm still open to adding a prop for alignment -- it would be easy enough & involve setting top: 0 or bottom: 0 depending on what user asks for.


Overshooting the size gets really wild

I understand what you are showing, but please consider that what you are demonstrating goes against the purpose of the props in this case? 😅 These props are being added to limit the number of lines shown to a small amount. If you are trying to show ALL options at once, then there's no point in modifying the prop.... or if you are trying to show MANY lines of options at once, well yes of course it will be as tall as you make it.

There's nothing fundamentally "incorrect" about the screenshot with the "overshooting". If you have five options in a select, and set the size attribute on the select to 100, then you are going to have a tall select 😅 And so forth.

@vpicone
Copy link
Contributor

vpicone commented Oct 25, 2019

@jendowns couldn't we chose the smaller of the two numbers: the set size prop and the actual number of pages?

Could this constraining just be solved with a max size? Rather than letting developers chose some arbitrary size they think looks right.

@jendowns
Copy link
Contributor Author

jendowns commented Oct 25, 2019

@vpicone In my experience, arbitrarily setting restrictions frustrates consumers 😅 What if you set it to 10 and someone wants 11, or 9? How would we determine that hard-coded magic number?

EDIT: What if the prop descriptions for itemCountSelectorSize & pageNumberSelectorSize included more info / recommendations? (Like, "these values set lines visible so if you set it very high, it will be tall" or something to that effect)

Could also include a dev console warning if itemCountSelectorSize or pageNumberSelectorSize is set to a high number? 🤔

@jendowns
Copy link
Contributor Author

@vpicone Just noticed this question here:

Could this constraining just be solved with a max size? Rather than letting developers chose some arbitrary size they think looks right.

If you are referring to setting this via a style, I don't recommend that. We are dealing with a select element here so they are notoriously difficult to customize 😅

If you are referring to setting a max via itemCountSelectorSize or pageNumberSelectorSize (so, the select's "size" attribute) then could be interesting... but again I wonder how you determine the max size in a way that makes everyone happy. To me it seems better to trust a developer to not do something extreme? But I can be convinced otherwise.

@vpicone
Copy link
Contributor

vpicone commented Oct 25, 2019

@jendowns I wasn't familiar with the size attribute on select, so thanks for bringing that up!

What about xSelectSize rather than xSelectorSize?

@jendowns
Copy link
Contributor Author

jendowns commented Oct 25, 2019

Yeah no worries @vpicone -- it was new to me too before I started looking into the originating issue 👀

About the prop name, @joshblack & I had a convo about it earlier in this collapsed thread: #4378 (comment)

Short version: consensus seems to be to keep the name more generic. So to me, using "selector" instead of "select" is more generic? (And doesn't reference the underlying select element explicitly, leaving it open to be changed to something else in the future) 😅 Plus "select" is also a verb so personally the more I look at any prop name with "select" in it, in this context, the murky the meaning gets...

@vpicone
Copy link
Contributor

vpicone commented Oct 25, 2019

@jendowns that makes sense, I really wish these were seperate components rather than needing to do everything through props!

I’m not sure the size attribute makes sense on any other element than select so I’m not sure the risk of it changing is all that high (could be wrong here, didn’t hear the convo with @joshblack). I think we should have the most accurate prop names we can now rather than optimizing for a future where we don’t use a select element.

Also Selector reminds me of css selectors 😂

@jendowns
Copy link
Contributor Author

@vpicone I'm totally okay with a prop name change, like:

  • itemCountSelectorSize -> itemCountSelectSize
  • pageNumberSelectorSize -> pageNumberSelectSize

I think @emyarod indicated he doesn't mind either way about the prop name?
But I would want to make sure that @joshblack is okay with this change, since he originally requested changes.

@jendowns
Copy link
Contributor Author

@joshblack @emyarod @vpicone @designertyler

I just renamed the following props:

  • itemCountSelectorSize -> itemCountSelectSize
  • pageNumberSelectorSize -> pageNumberSelectSize

I was wondering if anyone has anymore feedback about this PR? @joshblack I noticed you marked this as "changes requested" but I think at this point I've addressed your original points?

Just to get ahead of any questions resurfacing, in case someone missed the comments above -- here's my response to the feedback related to options overshooting the select if you pass in a big number (last part of the comment): #4378 (comment)

Please let me know if you have any more questions or concerns!

@emyarod
Copy link
Member

emyarod commented Oct 30, 2019

@jendowns I think we still need to come to a conclusion regarding @designertyler's point about dismissing the menu on select when the size attribute has a non-default value. but ultimately this sort of sidesteps the original issue which seems to require different UX (probably number input page selection) in order to avoid a large amount of <option>s in the DOM

I think we will explore the number input feature regardless, but in order to continue with exposing the select size attribute we will need to figure out how to deal with the menu dismiss behavior after selecting an option

@jendowns
Copy link
Contributor Author

jendowns commented Oct 30, 2019

I think we will explore the number input feature regardless, but in order to continue with exposing the select size attribute we will need to figure out how to deal with the menu dismiss behavior after selecting an option

@emyarod Agreed on the first point, this Pagination component does not cover some use cases from product that have many many pages. Maybe that solution ends up being a number input, maybe something else 🤔 Not sure (EDIT: just to be clear, this solution could work for many many pages, as long as the number of options in the select wouldn't theoretically crash the browser)

But to the second point about dismissing the options list when you select an option, I think that's the standard behavior of the select when you add a size attribute. And I don't think it's possible to dismiss the list without doing something strange. 🤔 Unless someone has an idea?

@joshblack
Copy link
Contributor

@jendowns do you still want to move forward with this btw? I know we talked a bit on Slack but wanted to double-check what you were hoping the outcome could be here. If it's continuing with this PR, definitely understand!

@jendowns
Copy link
Contributor Author

jendowns commented Nov 6, 2019

Oh @joshblack thanks for the reminder! Yeah I'll close this one out since the work we talked about would be taking place downstream in our repo first. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Page selector is not suitable for tables with many pages
6 participants