Skip to content

Conversation

ruairiphackett
Copy link
Contributor

@ruairiphackett ruairiphackett commented Apr 5, 2019

WHY are these changes introduced?

Fixes #1276

There's an issue with how the styling is applied to the previous and next buttons in the Pagination component.

The issue affects how margin-left and border-radius properties are applied to the buttons. Its assumed the buttons will be siblings, with the styling based on first-child and last-child.

As a result several styles aren't applied properly, for example, when a nextTooltip is specified, the next button gets styles for both first-child and last-child & doesn't receive the negative margin-left for :not(:first-child).

There's also similar issues when a previousTooltip is present or both tooltips.

Similarly there are issues with the plain buttons when they have tooltips.

Here are screenshots of different combinations of previousTooltip & nextTooltip before and after the changes in this PR are applied (code for button combinations below):

Before:

https://screenshot.click/2019-04-05_12-32-56.png

After:

https://screenshot.click/2019-04-05_12-33-54.png

WHAT is this pull request doing?

This PR adds classes specific to both the previous and next buttons. The reason new classes were introduced is that we couldn't properly target the buttons without them (we would have had to target previous and next siblings if we continued to just use the Button class).

How to 🎩

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

Copy-paste this code in playground/Playground.tsx:

Has different combinations of previous/next buttons with/without tooltips to highlight the different issues present

import React from 'react';
import {Pagination, Tooltip} from '@shopify/polaris';

export default class App extends React.Component {
  render() {
    return (
      <div>
        <Pagination
          hasPrevious
          onPrevious={() => {
            console.log('Previous');
          }}
          nextURL="https://github.com/Shopify/polaris-react"
          hasNext
          onNext={() => {
            console.log('Next');
          }}
        />
        <br />
        <br />
        <Pagination
          hasPrevious
          onPrevious={() => {
            console.log('Previous');
          }}
          hasNext
          onNext={() => {
            console.log('Next');
          }}
          nextTooltip="saadw"
        />
        <br />
        <br />
        <Pagination
          hasPrevious
          onPrevious={() => {
            console.log('Previous');
          }}
          previousTooltip="jkhk"
          hasNext
          onNext={() => {
            console.log('Next');
          }}
        />
        <br />
        <br />
        <Pagination
          hasPrevious
          onPrevious={() => {
            console.log('Previous');
          }}
          nextTooltip="wqweqewq"
          previousTooltip="uijii"
          hasNext
          onNext={() => {
            console.log('Next');
          }}
        />
        <br />
        <br />
        <Pagination
          hasPrevious
          onPrevious={() => {
            console.log('Previous');
          }}
          nextURL="https://github.com/Shopify/polaris-react"
          hasNext
          plain
          onNext={() => {
            console.log('Next');
          }}
        />
        <br />
        <br />
        <Pagination
          hasPrevious
          onPrevious={() => {
            console.log('Previous');
          }}
          hasNext
          plain
          onNext={() => {
            console.log('Next');
          }}
          nextTooltip="saadw"
        />
        <br />
        <br />
        <Pagination
          hasPrevious
          onPrevious={() => {
            console.log('Previous');
          }}
          previousTooltip="jkhk"
          hasNext
          plain
          onNext={() => {
            console.log('Next');
          }}
        />
        <br />
        <br />
        <Pagination
          hasPrevious
          onPrevious={() => {
            console.log('Previous');
          }}
          nextTooltip="wqweqewq"
          previousTooltip="uijii"
          hasNext
          plain
          onNext={() => {
            console.log('Next');
          }}
        />
      </div>
    );
  }
}

🎩 checklist

@ghost
Copy link

ghost commented Apr 5, 2019

👋 Thanks for opening your first pull request. A contributor should give feedback soon. If you haven’t already, please check out the contributing guidelines. You can also join #polaris on the Shopify Partners Slack.

@danrosenthal
Copy link

Hey @ruairiphackett, looks like a good start. Having some of my own experience dealing with connected buttons, I'm concerned about focus styles with the negative margin.

Can you post some screenshots of you tophatting this for focus behavior?

@ruairiphackett
Copy link
Contributor Author

ruairiphackett commented May 7, 2019

@danrosenthal Here's the behaviour when moving focus between the Pagination buttons: https://screenshot.click/2019-05-07_11-55-30.mp4

Let me know if it was something else you wanted me to show 👍

I went with the negative margin as it was being applied previously to the buttons, but was targeting elements incorrectly, I can use a different approach though, if it would be better 💯

@danrosenthal
Copy link

danrosenthal commented May 7, 2019

Nope, @ruairiphackett, that looks great! Just wanted to be sure 👍

The percy visual regression in SkeletonPage seems to be flakey. You can ignore that one and merge once you've rebased.

@ruairiphackett ruairiphackett force-pushed the pagination-prev-border branch from b36caa4 to 5c4886f Compare May 7, 2019 13:16
@ruairiphackett ruairiphackett merged commit e565e05 into master May 7, 2019
@ruairiphackett ruairiphackett deleted the pagination-prev-border branch May 7, 2019 13:23
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.

Previous button of the Pagination component has unnecessary right border

4 participants