Skip to content

Conversation

@thayannevls
Copy link
Contributor

@thayannevls thayannevls commented Apr 9, 2020

WHY are these changes introduced?

Fixes #2869

WHAT is this pull request doing?

Increasing content and wrapper max-height to 500px which is a great size for the tooltip in my opinion, If you guys don't consider this a good solution I'm open to change it.

Before
Screenshot from 2020-04-08 21-04-08

After
Screenshot from 2020-04-08 21-04-45

How to 🎩

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

Copy-paste this code in playground/Playground.tsx:
import React from 'react';

import {Page} from '../src';
import {Tooltip, Link} from '../src'

export function Playground() {
  return (
    <Page title="Playground">
      {/* Add the code you want to test in here */}
      <Tooltip
        content="this is a loooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooong text"
      >
        <Link>Order #1001</Link>
      </Tooltip>
    </Page>
  );
}

@ghost
Copy link

ghost commented Apr 9, 2020

👋 Thanks for opening your first pull request. A contributor should give feedback soon. If you haven’t already, please check out the contributing guidelines.

@ghost ghost added the cla-needed Added by a bot. Contributor needs to sign the CLA Agreement. label Apr 9, 2020
@thayannevls thayannevls force-pushed the tooltip-label-length branch from d2d1bbd to 3a01ea8 Compare April 9, 2020 00:19
@ghost ghost removed the cla-needed Added by a bot. Contributor needs to sign the CLA Agreement. label Apr 9, 2020
@thayannevls thayannevls changed the title [Tooltip] Increase content max-height [Tooltip] Remove content max-height Apr 13, 2020
@sylvhama sylvhama requested review from kyledurand and sylvhama April 13, 2020 20:47
Copy link
Member

@kyledurand kyledurand left a comment

Choose a reason for hiding this comment

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

Code looks good, just make sure to rebase against the latest master. Looks like there's a conflict in unreleased.

Thanks for this 👍

Copy link
Contributor

@sylvhama sylvhama left a comment

Choose a reason for hiding this comment

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

LGTM

@thayannevls thayannevls force-pushed the tooltip-label-length branch from c2d0989 to 7adf6e2 Compare April 13, 2020 20:55
@thayannevls
Copy link
Contributor Author

The build failed because of changes made in master

UNRELEASED.md Outdated
### Enhancements

- Updated `Filters` to only show the "More filters" button if necessary ([#2856](https://github.com/Shopify/polaris-react/pull/2856)).
- Added utilities for parsing video duration (https://polaris.shopify.com/components/images-and-icons/video-thumbnail) ([#2725](https://github.com/Shopify/polaris-react/pull/2725))
Copy link
Member

Choose a reason for hiding this comment

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

This line can be removed. It already exists in the changelog which is why your build is failing. You might need to remove this line and rebase again 😅

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'm sorry, something went wrong with my rebase. Just fixed it 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

There is another conflict in UNRELEASED.md 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed again 😅

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we are very unlucky today there is again a conflict 😢

Copy link
Member

Choose a reason for hiding this comment

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

Let me know if you want me to get this over the finish line @thayannevls

Happy to help, and sorry about all of the conflicts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@shioyama Yes 😢 I fixed again lol

@kyledurand Do you think we can already merge this PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

Now the CI is failing because of duplicate-changelog.test.js, let me know if I can help to fix that.

@thayannevls thayannevls force-pushed the tooltip-label-length branch from 7adf6e2 to d01d8e8 Compare April 14, 2020 17:10
@carolopolo carolopolo removed their request for review April 14, 2020 17:19
@thayannevls thayannevls force-pushed the tooltip-label-length branch from d01d8e8 to 39e1964 Compare April 15, 2020 15:58
@thayannevls thayannevls force-pushed the tooltip-label-length branch from 39e1964 to a305167 Compare April 15, 2020 19:03
@sylvhama
Copy link
Contributor

@thayannevls it seems the CI has some problems, I will follow up with Polaris people to find a solution!

@alex-page alex-page merged commit c45bf48 into Shopify:master Apr 16, 2020
@ghost
Copy link

ghost commented Apr 16, 2020

🎉 Thanks for your contribution to Polaris React!

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.

Tooltip with long text seems t have no padding-bottom

5 participants