Skip to content
This repository was archived by the owner on Jul 12, 2024. It is now read-only.

Add showPagePicker, showPerPagePicker props to Pagination component#3980

Merged
findingsimple merged 5 commits intomasterfrom
add/pagination-options
Mar 27, 2020
Merged

Add showPagePicker, showPerPagePicker props to Pagination component#3980
findingsimple merged 5 commits intomasterfrom
add/pagination-options

Conversation

@danielbitzer
Copy link
Copy Markdown
Contributor

@danielbitzer danielbitzer commented Mar 24, 2020 •
edited by findingsimple
Loading

I would like to use the Pagination component in the slider I'm working on for the marketing tab (#3953). Rather than duplicate this functionality we can gain a lot by using the existing Pagination component. All we need is to hide the parts we don't need.

This this RP adds 2 new props showPagePicker and showPerPagePicker which remove parts of the pagination component that I don't want for the marketing slider.

Screenshot

The PR adds a story for the Pagination component in 2577879 which you can use to try out the new properties.

60NOwo

Accessibility

  • I've tested using only a keyboard (no mouse)
  • I've tested using a screen reader
  • All animations respect prefers-reduced-motion
  • All text has [at least a 4.5 color contrast with its background](https://webaim.org

Detailed test instructions:

  • Fire up storybook e.g. npm run storybook
  • Go to the Pagination component
  • Enable/disable pickers using the checkboxes for "Page Picker" and "Per Page Picker"

@danielbitzer danielbitzer added the type: enhancement The issue is a request for an enhancement. label Mar 24, 2020
@probot-autolabeler probot-autolabeler bot added focus: components Issues for woocommerce components Packages labels Mar 24, 2020
@findingsimple findingsimple added the focus: marketing Related to Marketing Features label Mar 24, 2020
psealock
psealock reviewed Mar 24, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@psealock psealock left a comment

Choose a reason for hiding this comment

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

@danielbitzer this is fantastic and a great way to reuse functionality!

Generally, we provide test instructions for reviewers and add the "Needs review" tag if its ready for review. Since the marketing material is harder for testers, I think this would be a great opportunity to contribute to our move to Storybook for documentation.

If you create a stories folder, ie https://github.com/woocommerce/woocommerce-admin/tree/master/packages/components/src/rating/stories, then npm run storybook will render the component and allow change of props for testing. Is this all thats needed @haszari?

@haszari
Copy link
Copy Markdown
Contributor

haszari commented Mar 24, 2020

If you create a stories folder, ie https://github.com/woocommerce/woocommerce-admin/tree/master/packages/components/src/rating/stories, then npm run storybook will render the component and allow change of props for testing.

Great idea, sounds right to me @psealock !

@danielbitzer Ping me if you get stuck with this, we have good time zone crossover.

Also should be mentioned – in blocks we now have storybook built & published on every push to master (check it out here), looks like it's fairly simple to set up (props @nerrad). Would be good to add similar to this repo.

@findingsimple
Copy link
Copy Markdown
Contributor

findingsimple commented Mar 25, 2020

👋 @psealock I've:

  • added a Story with 2577879
  • updated the PR description to reflect the additional change
  • added test instructions

psealock
psealock reviewed Mar 25, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@psealock psealock left a comment

Choose a reason for hiding this comment

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

Thanks for the update @jconroy! Looking fantastic.

Should we add another prop to hide/show "Page 2 of 10"? Are you using CSS to hide it in the banner?

@danielbitzer
Copy link
Copy Markdown
Contributor Author

danielbitzer commented Mar 26, 2020 •
edited
Loading

Should we add another prop to hide/show "Page 2 of 10"? Are you using CSS to hide it in the banner?

I considered this. What would be you preference @psealock? If it existed we would use it (currently we use CSS).

@findingsimple findingsimple force-pushed the add/pagination-options branch from 54023cd to 504369a Compare March 26, 2020 04:59
@findingsimple
Copy link
Copy Markdown
Contributor

findingsimple commented Mar 26, 2020

Went ahead and added a showPageArrowsLabel prop.

Also rebased on master to pick up the storybook fix in #4001

@findingsimple findingsimple mentioned this pull request Mar 26, 2020
Merged
@octaedro octaedro self-requested a review March 26, 2020 18:16
psealock
psealock approved these changes Mar 26, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@psealock psealock left a comment

Choose a reason for hiding this comment

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

Looks great! 🚢

octaedro
octaedro approved these changes Mar 26, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@octaedro octaedro left a comment

Choose a reason for hiding this comment

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

LGTM :shipit:

@findingsimple findingsimple merged commit 8484150 into master Mar 27, 2020
@findingsimple findingsimple deleted the add/pagination-options branch March 27, 2020 00:54
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@octaedro octaedro octaedro approved these changes

@psealock psealock psealock approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

focus: components Issues for woocommerce components focus: marketing Related to Marketing Features type: enhancement The issue is a request for an enhancement.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@danielbitzer @haszari @findingsimple @octaedro @psealock