Skip to content

Conversation

@mitchmaps
Copy link
Contributor

Why are these changes introduced?

Addresses feature request #1760 following guideline set here.

What is this pull request doing?

TL;DR: Adds a prop to insert text in between the backwards and forwards pagination buttons.

This PR addresses an issue opened by @Tetsuro who had created a version of the change for Shopify/online-store-ui and wanted to see the feature he built introduced into the standard Polaris react library.

The new label prop aims to provide users more context for what the pagination component is representing by adding descriptive text in between the backwards and forwards button. The example @Tetsuro used in the original issue was text indicating how many pages there were to cycle between but the prop could be used for anything a user wants.

Example of a Pagination component using the new label prop

Desktop
image

Mobile
image

How to 🎩

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

  1. Checkout the branch
  2. Run the server
  3. Add a Pagination component to the Playground.tsx file and pass in a label prop
  4. Checkout playground on the server and check out the visual changes to the component
Copy-paste this code in playground/Playground.tsx:
import React from 'react';
import {Page} from '../src';

export function Playground() {
  return (
    <Page title="Playground">
      <Pagination label="1 of 10" />
    </Page>
  );
}

🎩 checklist

@mitchmaps mitchmaps requested a review from Tetsuro September 9, 2019 14:32
@mitchmaps mitchmaps force-pushed the add-label-prop-to-pagination branch from 84c6e9d to ec5005e Compare September 9, 2019 14:43
@dleroux
Copy link
Contributor

dleroux commented Sep 16, 2019

@sarahill ui-kit updates?

@perrupa
Copy link
Contributor

perrupa commented Sep 16, 2019

Looks like the next and previous button corners aren't rounded as I'd expect when there's a label (when they don't have adjacent buttons). Is that intentional?

@sarahill sarahill self-requested a review September 16, 2019 19:50
Copy link
Contributor

@sarahill sarahill left a comment

Choose a reason for hiding this comment

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

Looking good! Agree with @perrupa that all corners should be rounded when text is separating the buttons. Feels a little off when they're square on one side.

@mitchmaps mitchmaps requested a review from dleroux September 18, 2019 14:02
@mitchmaps
Copy link
Contributor Author

mitchmaps commented Sep 18, 2019

@sarahill and @perrupa 👀

FIXED: Buttons with a label having rounded corners

image

Previous style without a label preserved:

image

@mitchmaps mitchmaps requested a review from sarahill September 18, 2019 14:18
Copy link
Contributor

@perrupa perrupa left a comment

Choose a reason for hiding this comment

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

If we change the css class logic, we can burn some extra CSS and code.

Left a few other suggestions, but I think the CSS/class logic is the only change I'd consider blocking (though a subdued test would be nice-to-have 😉 )

@perrupa
Copy link
Contributor

perrupa commented Sep 18, 2019

Also, might want to add it to the changelog ❤️

.find('.Label')
.children()
.prop('variation'),
).toStrictEqual(variation);
Copy link
Contributor

@perrupa perrupa Sep 18, 2019

Choose a reason for hiding this comment

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

I had a couple of ideas for this one 😄

Do you need .find('.Label').children() over .find('.TextStyle'), or was that not working?

Also, is there a reason you put these values in variables? I think the test might read a bit clearer with less indirection, what do you think?

const label = mountWithAppProvider(<Pagination label="label" />).find('.TextStyle');
expect(label.prop('variation')).toStrictEqual('subdued');

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried doing it with .Textstyle and it was running into some issues finding the specific child to compare to. Using my own class from the label component seemed to do the trick but if there's a better way to go about it then I think it's something worth changing.

And I agree that might be a little overkill for the variables if they're only used once, a line after they were defined. I think I just put those in to follow the pattern of creating a variable for the component itself to assert.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changes have been made 🔥

Copy link
Contributor

@sarahill sarahill left a comment

Choose a reason for hiding this comment

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

The rounded corners look good ✨

Make sure to add this as an example in the components README.md file along with a description of its usage so it gets added to the Style guide. cc: @sadiesaurus

Copy link
Contributor

@perrupa perrupa left a comment

Choose a reason for hiding this comment

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

Dope work! 👍

*Just looks like you've gotta ok that visual change in percy, I think it should be good-to-go after that? 😄

@mitchmaps mitchmaps merged commit 0bdea25 into master Sep 23, 2019
@mitchmaps mitchmaps deleted the add-label-prop-to-pagination branch September 23, 2019 14:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants