Skip to content

Conversation

devonpmack
Copy link
Contributor

@devonpmack devonpmack commented Feb 14, 2020

WHY are these changes introduced?

Fixes #2745

WHAT is this pull request doing?

Properly styling the buttons even if there is a tooltip wrapper on one of them

Before After
image image

How to 🎩

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

Make sure you resize your window to small width (mobile)

Copy-paste this code in playground/Playground.tsx:
import React from 'react';
import {Page, Pagination} from '../src';

export function Playground() {
  return (
    <Page title="Playground">
      <div>
        <Pagination hasPrevious previousTooltip="previous" />
        <br />
        <Pagination hasPrevious />
      </div>
    </Page>
  );
}

🎩 checklist

  • Tested on mobile
  • Tested on multiple browsers
  • Tested for accessibility
  • Updated the component's README.md with documentation changes
  • Tophatted documentation changes in the style guide
  • For visual design changes, pinged one of @ HYPD, @ mirualves, @ sarahill, or @ ry5n to update the Polaris UI kit

Comment on lines +32 to +35
.Button {
width: 100%;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Getting a linting error because it's doing 3 levels of nesting. Anyone know a better way to do this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Because the span will get added inside the other component I think this is your best bet. Good job on explicitly adding the span in the prop, it avoids surprises.

@github-actions
Copy link
Contributor

github-actions bot commented Feb 14, 2020

🟢 This pull request modifies 3 files and might impact 3 other files.

Details:
All files potentially affected (total: 3)
📄 UNRELEASED.md (total: 0)

Files potentially affected (total: 0)

🎨 src/components/Pagination/Pagination.scss (total: 3)

Files potentially affected (total: 3)

🧩 src/components/Pagination/Pagination.tsx (total: 2)

Files potentially affected (total: 2)

@devonpmack devonpmack force-pushed the pagination/fix-small-screen-size branch from 8a9b4bd to c2899a7 Compare February 24, 2020 20:17
@devonpmack devonpmack marked this pull request as ready for review February 24, 2020 20:17
@devonpmack devonpmack changed the title [Pagination] Fix sizing on small screens [Pagination] Fix sizing on small screens when they have tooltips Feb 24, 2020
const constructedPrevious =
previousTooltip && hasPrevious ? (
<Tooltip content={previousTooltip}>{previousButton}</Tooltip>
<Tooltip activatorWrapper="span" content={previousTooltip}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice that you specify the wrapper!

Copy link
Contributor

@PabloVallejo PabloVallejo left a comment

Choose a reason for hiding this comment

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

🎩 Working nicely in small resolution! ✅
image

@devonpmack devonpmack force-pushed the pagination/fix-small-screen-size branch from c2899a7 to 3040843 Compare February 26, 2020 19:55
@devonpmack devonpmack merged commit 446f700 into master Feb 26, 2020
@devonpmack devonpmack deleted the pagination/fix-small-screen-size branch February 26, 2020 20:16
@tmlayton tmlayton temporarily deployed to production March 7, 2020 05:24 Inactive
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.

[Pagination] Buttons don't resize correctly on small screen when they have a tooltip

4 participants