feat(tooltip): allow tooltip text to be left-aligned by jenny-s51 · Pull Request #3053 · patternfly/patternfly-react · GitHub

will close #3013

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

Codecov Report

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

@@            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.

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

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.

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?

@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.

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.

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.

@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?

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

@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 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.

I see no reason to support right-aligned tooltips.

LGTM, unused import doesn't affect anything.

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.

All set! #3088

Unused import.

lgtm, thanks @jenny-s51!!

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

@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?

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.

jenny-s51 added the Do Not Merge label Oct 1, 2019

jenny-s51 force-pushed the iss3013 branch from b3291a3 to eef83db Compare October 3, 2019 00:39

jenny-s51 removed the Do Not Merge label Oct 3, 2019

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 requested review from dtaylor113, redallen and tlabaj October 3, 2019 00:41

dlabrecq suggested changes Oct 4, 2019

View reviewed changes

jenny-s51 added 5 commits October 5, 2019 14:19

WIP(tooltip): adding left-align prop and functionality

c5aa091

adds missing default prop

f9ca939

renaming prop

596d2cf

enhancement(tooltip): allows content to be left-aligned

2719b37

add prop to Tooltip instance

f6422a0

jenny-s51 force-pushed the iss3013 branch from eef83db to f6422a0 Compare October 5, 2019 18:29

adds updated snapshot tests

c282699

jenny-s51 added the Hacktoberfest 🎃 label Oct 5, 2019

jenny-s51 requested a review from dlabrecq October 5, 2019 18:58

redallen approved these changes Oct 7, 2019

View reviewed changes

This was referenced Oct 7, 2019

Tooltip: Provide examples on how/why to use TooltipContent #3088

Closed

PF4 Tooltip: add support for right-aligned content #3089

Closed

tlabaj added the css review label Oct 8, 2019

tlabaj requested a review from mcoker October 8, 2019 15:04

mcoker approved these changes Oct 8, 2019

View reviewed changes

jenny-s51 removed the css review label Oct 11, 2019

tlabaj self-assigned this Oct 11, 2019

dlabaj requested changes Oct 11, 2019

View reviewed changes

jenny-s51 requested a review from dlabaj October 11, 2019 16:22

dlabaj approved these changes Oct 11, 2019

View reviewed changes

dlabrecq approved these changes Oct 11, 2019

View reviewed changes

dlabrecq merged commit d369c07 into patternfly:master Oct 11, 2019

                 /** Tooltip trigger: click, mouseenter, focus, manual  */
                 trigger?: string;
+                /** Flag to indicate that the text content is left aligned */
+                isContentLeftAligned?: boolean;

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

                 /** Tooltip trigger: click, mouseenter, focus, manual  */
                 trigger?: string;
+                /** Flag to indicate that the text content is left aligned */
+                isContentLeftAligned?: boolean;

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

feat(tooltip): allow tooltip text to be left-aligned by jenny-s51 · Pull Request #3053 · patternfly/patternfly-react · GitHub

Uh oh!

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

feat(tooltip): allow tooltip text to be left-aligned#3053dlabrecq merged 6 commits intopatternfly:masterpatternfly/patternfly-react:masterfrom jenny-s51:iss3013jenny-s51/patternfly-react:iss3013Copy head branch name to clipboard

Conversation

jenny-s51 commented Oct 1, 2019

Uh oh!

patternfly-build commented Oct 1, 2019

Uh oh!

codecov-io commented Oct 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

redallen commented Oct 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dlabrecq Oct 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jenny-s51 Oct 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dlabrecq Oct 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jenny-s51 Oct 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

redallen left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcoker left a comment

Choose a reason for hiding this comment

Uh oh!

dlabaj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dlabaj left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

feat(tooltip): allow tooltip text to be left-aligned#3053
dlabrecq merged 6 commits into
patternfly:masterfrom
jenny-s51:iss3013

codecov-io commented Oct 3, 2019 •
edited
Loading

redallen commented Oct 3, 2019 •
edited
Loading

dlabrecq Oct 4, 2019 •
edited
Loading

jenny-s51 Oct 5, 2019 •
edited
Loading

dlabrecq Oct 8, 2019 •
edited
Loading

jenny-s51 Oct 10, 2019 •
edited
Loading

redallen left a comment •
edited
Loading