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

fix(pagination): add pageSizeInputDisabled prop to Pagination #6070

Merged
merged 12 commits into from
Jun 12, 2020

Conversation

tw15egan
Copy link
Collaborator

@tw15egan tw15egan commented May 13, 2020

Closes #6055

This PR now ties all Pagination inputs / buttons to the disabled prop, rather than having 3 separate props ton control them individually. This also deprecates the pageInputDisabled prop.

Changelog

Changed

  • disabled prop now controls both select elements, as well as the back and forward buttons.
  • pageInputDisabled prop now will throw depraction warnings

Testing / Reviewing

Go to Pagination, enable the disabled prop, and make sure all inputs are disabled

@tw15egan tw15egan requested a review from a team as a code owner May 13, 2020 15:36
@ghost ghost requested review from asudoh and emyarod May 13, 2020 15:36
@tw15egan tw15egan requested review from a team and aagonzales and removed request for a team May 13, 2020 15:37
@netlify
Copy link

netlify bot commented May 13, 2020

Deploy preview for carbon-elements ready!

Built with commit b75639c

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

@netlify
Copy link

netlify bot commented May 13, 2020

Deploy preview for carbon-components-react ready!

Built with commit b75639c

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

Copy link
Member

@aagonzales aagonzales left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍

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.

the prop name in the knobs section is the same as the other input knob

@joshblack
Copy link
Contributor

Looks great after Andrew's suggestions above!

@tw15egan
Copy link
Collaborator Author

@emyarod good catch, updated 👍

@tw15egan tw15egan requested a review from emyarod May 21, 2020 15:57
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.

are there instances where each input is disabled separately from the others or should the logic be combined for certain component states?

@tw15egan
Copy link
Collaborator Author

I'm not sure of the total use cases when the pagination is disabled, so I didn't want to combine any of the disabling in case they are used in different scenarios.

I think eventually we may want to go that route, but might be best to wait until v11 and have them all use the disabled attribute?

@emyarod
Copy link
Member

emyarod commented May 21, 2020

just looking for clarification on the component usage before adding the extra prop, since if it is supposed to just be linked with the regular disabled prop we would be adding in a new prop now and then deprecating it in v11 right?

@joshblack
Copy link
Contributor

bump @tw15egan if you have a sec to respond to the last comment 👀

@tw15egan
Copy link
Collaborator Author

I don't have any insight into that, but if you think we should just tie the disabled states together I'm fine with that 👍

@emyarod
Copy link
Member

emyarod commented May 27, 2020

yeah, similar to #6112 (comment) I think if the disabled states are meant to be linked then it makes sense not to expand the API surface area

@tw15egan tw15egan force-pushed the page-size-disabled branch from 0e1422f to 95c351b Compare June 9, 2020 15:50
@tw15egan tw15egan requested review from joshblack and aagonzales June 9, 2020 15:52
@tw15egan
Copy link
Collaborator Author

tw15egan commented Jun 9, 2020

@emyarod @joshblack @aagonzales I updated this PR to tie all inputs to the disabled prop and added a deprecation warning to pageInputDisabled prop.

Copy link
Member

@aagonzales aagonzales left a comment

Choose a reason for hiding this comment

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

Cool, yeah I'm not sure how people are using the disabled features with pagination. But one way to find out is to change this and see if anyone complains. I think it should be fine though I can't really image why some might be disabled but not others. Seems like products would use this disabled feature mostly for when there isn't any page to paginate to.

@tw15egan
Copy link
Collaborator Author

tw15egan commented Jun 9, 2020

@aagonzales agreed, I reached out to the original issue creator and they agreed with that assumption as well

Screen Shot 2020-06-09 at 3 23 53 PM

#6055 (comment)

@tw15egan tw15egan requested a review from emyarod June 12, 2020 11:53
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.

looks good to me

@s100
Copy link
Contributor

s100 commented Jun 18, 2020

Following this change, it seems like it's now impossible to use <Pagination> without raising a deprecation warning. If we set the pageInputDisabled prop to true or false, we get the warning. If we set it to null or undefined, or omit it entirely, it defaults to false and we get the warning. This is a problem for us because our unit tests fail if warnings or errors are raised. Any suggestions?

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.

Not possible to disable Pagination pagesize input
5 participants