Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(tooltip): allow tooltip text to be left-aligned #3053

Merged
merged 6 commits into from Oct 11, 2019

Conversation

@jenny-s51
Copy link
Contributor

jenny-s51 commented Oct 1, 2019

will close #3013

@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Oct 1, 2019

PatternFly-React preview: https://patternfly-react-pr-3053.surge.sh

@jenny-s51 jenny-s51 force-pushed the jenny-s51:iss3013 branch from b3291a3 to eef83db Oct 3, 2019
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Oct 3, 2019

Codecov Report

Merging #3053 into master will decrease coverage by <.01%.
The diff coverage is 75%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3053      +/-   ##
==========================================
- Coverage   69.02%   69.02%   -0.01%     
==========================================
  Files         858      858              
  Lines       23413    23416       +3     
  Branches     1853     1854       +1     
==========================================
+ Hits        16160    16162       +2     
- Misses       6333     6334       +1     
  Partials      920      920
Flag Coverage Δ
#misc 95.45% <ø> (ø) ⬆️
#patternfly3 69.22% <ø> (ø) ⬆️
#patternfly4 68.09% <75%> (-0.01%) ⬇️
Impacted Files Coverage Δ
...ly-4/react-core/src/components/Tooltip/Tooltip.tsx 84.48% <100%> (+0.27%) ⬆️
...mponents/TopologyControlBar/TopologyControlBar.tsx 45.12% <100%> (ø) ⬆️
...act-core/src/components/Tooltip/TooltipContent.tsx 80% <50%> (-20%) ⬇️
...patternfly-react/src/components/Tooltip/Tooltip.js 94.18% <0%> (+0.06%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 650c823...c282699. Read the comment docs.

@jenny-s51 jenny-s51 removed the Do Not Merge label Oct 3, 2019
@jenny-s51 jenny-s51 changed the title WIP(tooltip): allow tooltip text to be left-aligned feat(tooltip): allow tooltip text to be left-aligned Oct 3, 2019
@jenny-s51 jenny-s51 requested review from redallen, tlabaj and dtaylor113 Oct 3, 2019
@redallen

This comment has been minimized.

Copy link
Contributor

redallen commented Oct 3, 2019

Very close to CI passing:

Snapshot Summary
 › 2 snapshots failed from 1 test suite. Inspect your code changes or run `yarn run test:pf4 -u` to update them.

Test Suites: 1 failed, 157 passed, 158 total
Tests:       2 failed, 849 passed, 851 total
Snapshots:   2 failed, 621 passed, 623 total
@@ -61,6 +61,8 @@ export interface TooltipProps {
position?: 'auto' | 'top' | 'bottom' | 'left' | 'right';
/** Tooltip trigger: click, mouseenter, focus, manual */
trigger?: string;
/** Flag to indicate that the text content is left aligned */
isContentLeftAligned?: boolean;

This comment has been minimized.

Copy link
@dlabrecq

dlabrecq Oct 4, 2019

Member

Why only support a left aligned option? What if designers want to support right-aligned content, next?

Even if we don't support right-aligned content for now, something more like this would scale better in the future.

contentPosition?: 'bottom' | 'center' | 'right';

Then we could create an enum

export enum TooltipContentPosition {
  bottom = 'bottom',
  center = 'center',
  right = 'right'
}

Looks like we already take this approach for the position property.

This comment has been minimized.

Copy link
@jenny-s51

jenny-s51 Oct 5, 2019

Author Contributor

Sure, I'm happy to add a right-aligned option. I was also wondering why this issue was opened for left-aligned text only. @dlabrecq @tlabaj would you prefer I open a new issue to add support for right-aligned content? Or add an enum to this PR instead, like you have above?

This comment has been minimized.

Copy link
@jenny-s51

jenny-s51 Oct 7, 2019

Author Contributor

@dlabrecq @tlabaj I've opened a separate issue for this at #3089. If agreeable with you, I'm happy to assign myself to it and add support right-aligned content as well.

This comment has been minimized.

Copy link
@dlabrecq

dlabrecq Oct 8, 2019

Member

I wasn't necessarily asking for right-aligned tooltips, just that we use a property that is more scalable. Once we introduce the isContentLeftAligned prop, it will be difficult to change without a breaking change. That's why I suggested we use something more like the existing position property.

I just want to consider a use case where we end up with both isContentLeftAligned and isContentRightAligned props together? Or, would it be better to have something more like contentPosition?: 'bottom' | 'center' | 'right' where we can easily add future support.

This comment has been minimized.

Copy link
@tlabaj

tlabaj Oct 10, 2019

Contributor

Dan makes good point here. I don't think there are plans of introducing the right aligned variant and the naming here seems to be consistent with other components. That being said those components would be hard to change if we added another variant.

This comment has been minimized.

Copy link
@jenny-s51

jenny-s51 Oct 10, 2019

Author Contributor

@tlabaj Thank you for your feedback! Does that mean I should change the prop to look like contentPosition?: 'left' | 'center'? Or did you mean that this PR is okay the way it is?

This comment has been minimized.

Copy link
@dlabrecq

dlabrecq Oct 11, 2019

Member

If you don't feel we'll ever have another variant, I'm fine leaving it as is. Just wanted to raise the question.

This comment has been minimized.

Copy link
@dlabaj

dlabaj Oct 11, 2019

Contributor

@jenny-s51 I think we should use contentPosition?: 'left' | 'center'. It's also consistent with the position enum we are already using in this component. Can we change it to that?

This comment has been minimized.

Copy link
@tlabaj

tlabaj Oct 11, 2019

Contributor

@jenny-s51 I like what @dlabrecq suggested. But, if we are not going to have a right alignment variation I don't see the need to introduce the inconstancy. We have at least one other component that uses isLeftAligned (or similar).
@mcarrano can you verify that there is no intent on introducing a right aligned variant.

This comment has been minimized.

Copy link
@mcarrano

mcarrano Oct 11, 2019

Member

I see no reason to support right-aligned tooltips.

@jenny-s51 jenny-s51 force-pushed the jenny-s51:iss3013 branch from eef83db to f6422a0 Oct 5, 2019
@jenny-s51 jenny-s51 requested a review from dlabrecq Oct 5, 2019
Copy link
Contributor

redallen left a comment

LGTM, unused import doesn't affect anything.

@@ -2,10 +2,10 @@
title: 'Tooltip'
cssPrefix: 'pf-c-tooltip'
typescript: true
propComponents: ['Tooltip']
propComponents: ['Tooltip', 'TooltipContent']

This comment has been minimized.

Copy link
@redallen

redallen Oct 7, 2019

Contributor

Good catch, we export TooltipContent in Tooltip/index.ts but don't show its props in the docs.

Edit: We should add a followup issue to provide examples on how/why to use TooltipContent.

This comment has been minimized.

Copy link
@jenny-s51

jenny-s51 Oct 7, 2019

Author Contributor

All set! #3088

---

import { Button, Tooltip, TooltipPosition, Checkbox } from '@patternfly/react-core';
import { Button, Tooltip, TooltipPosition, TooltipContent, Checkbox } from '@patternfly/react-core';

This comment has been minimized.

Copy link
@redallen

redallen Oct 7, 2019

Contributor

Unused import.

@mcoker
mcoker approved these changes Oct 8, 2019
Copy link
Contributor

mcoker left a comment

lgtm, thanks @jenny-s51!!

@jenny-s51 jenny-s51 removed the css review label Oct 11, 2019
@tlabaj tlabaj self-assigned this Oct 11, 2019
Copy link
Contributor

dlabaj left a comment

Can you update this to use contentPosition?: 'left' | 'center'`

@@ -61,6 +61,8 @@ export interface TooltipProps {
position?: 'auto' | 'top' | 'bottom' | 'left' | 'right';
/** Tooltip trigger: click, mouseenter, focus, manual */
trigger?: string;
/** Flag to indicate that the text content is left aligned */
isContentLeftAligned?: boolean;

This comment has been minimized.

Copy link
@dlabaj

dlabaj Oct 11, 2019

Contributor

@jenny-s51 I think we should use contentPosition?: 'left' | 'center'. It's also consistent with the position enum we are already using in this component. Can we change it to that?

@jenny-s51 jenny-s51 requested a review from dlabaj Oct 11, 2019
@dlabaj
dlabaj approved these changes Oct 11, 2019
Copy link
Contributor

dlabaj left a comment

After looking at what we do elsewhere it seems we should go with IsContentLeftAligned. We only seem to use the enum when there are more than one style, and we are using IsLeftAligned elsewhere through out the product.

@dlabrecq dlabrecq merged commit d369c07 into patternfly:master Oct 11, 2019
8 checks passed
8 checks passed
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: build_integration Your tests passed on CircleCI!
Details
ci/circleci: build_pf3_docs Your tests passed on CircleCI!
Details
ci/circleci: build_pf4_docs Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: test_jest_other Your tests passed on CircleCI!
Details
ci/circleci: test_jest_pf4 Your tests passed on CircleCI!
Details
ci/circleci: upload_docs Your tests passed on CircleCI!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.