feat(tooltip): allow custom CSS classes for displaying tooltips #4016

emyarod · 2019-09-18T03:02:43Z

Closes #4015

This PR extracts the tooltip display code in a new mixin called tooltip--visible()

Changelog

New

tooltip--visible() Sass mixin containing style rules needed to display definition and icon tooltips

Changed

refactor tooltip positioning selectors to remove unneeded :hover and :focus selectors

Testing / Reviewing

Create a new tooltip example with test classes (in this example, it is .bx--test and .bx--test--focus)

<div class="{{@root.prefix}}--tooltip--definition {{@root.prefix}}--tooltip--a11y">
  <button
    class="{{@root.prefix}}--tooltip__trigger {{@root.prefix}}--tooltip--a11y {{@root.prefix}}--tooltip--left {{@root.prefix}}--tooltip--align-start">
    <span class="{{@root.prefix}}--assistive-text">Filter</span>
    {{ carbon-icon 'Filter16' }}
  </button>
  <span aria-describedby="test"
    class="{{@root.prefix}}--test {{@root.prefix}}--tooltip--a11y {{@root.prefix}}--test--focus">
    Definition Tooltip (open)
  </span>
  <div class="bx--assistive-text" id="test" role="tooltip">Brief description of the dotted, underlined word
    above.</div>
</div>

apply the default tooltip mixins and the new tooltip mixin to the test classes

.#{$prefix}--test {
  @include tooltip--trigger('definition', 'bottom');
  @include tooltip--placement('definition', 'bottom', 'center');
}

.#{$prefix}--test--focus {
  @include tooltip--visible();
}

ensure that the tooltip is open by default and ensure existing tooltip implementation is not broken

emyarod · 2019-09-18T03:06:36Z

cc @elizabethsjudd due to overlap with #3115 #3151

netlify · 2019-09-18T03:10:53Z

Deploy preview for the-carbon-components ready!

Built with commit b64f389

https://deploy-preview-4016--the-carbon-components.netlify.com

netlify · 2019-09-18T03:14:02Z

Deploy preview for carbon-components-react ready!

Built with commit b64f389

https://deploy-preview-4016--carbon-components-react.netlify.com

netlify · 2019-09-18T03:14:59Z

Deploy preview for carbon-elements ready!

Built with commit b64f389

https://deploy-preview-4016--carbon-elements.netlify.com

asudoh

🎉 - Thanks @emyarod!

@elizabethsjudd We are making a change to ally style rules like https://github.com/carbon-design-system/carbon/blob/v10.6.0/packages/components/src/globals/scss/_tooltip.scss#L92-L107 regardless of tooltip's active state, wanted to make sure it won't cause any adverse effect to what you implemented. Thanks!

packages/components/src/globals/scss/_tooltip.scss

netlify · 2019-09-18T03:21:59Z

Deploy preview for the-carbon-components ready!

Built with commit 363fb0a

https://deploy-preview-4016--the-carbon-components.netlify.com

netlify · 2019-09-18T03:27:39Z

Deploy preview for carbon-components-react ready!

Built with commit 363fb0a

https://deploy-preview-4016--carbon-components-react.netlify.com

netlify · 2019-09-18T03:28:53Z

Deploy preview for carbon-elements ready!

Built with commit 363fb0a

https://deploy-preview-4016--carbon-elements.netlify.com

joshblack

I feel like this is a larger change that we should first decide on in the issue before bringing on an implementation as this heavily ties into tooltips and our ability to make sure they're accessible. I think there are also other approaches we might be able to take on here if we need a generalized tooltip component for these cases

emyarod · 2019-09-19T01:47:48Z

Let’s go with a staged approach to help read-only text input go forward

Slight API change that helps achieve the read-only text input
re-examination of the feature set and architecture of the tooltip as a whole afterwards

vpicone · 2019-09-19T02:24:57Z

packages/components/src/globals/scss/_tooltip.scss

-  &:hover + .#{$prefix}--assistive-text,
-  &:focus + .#{$prefix}--assistive-text {
+  .#{$prefix}--assistive-text,
+  + .#{$prefix}--assistive-text {


Can you help me understand what this sibling selector is doing?

prior to #3115 and #3151 we used ::after pseudoelements to create the tooltip body. this pattern was changed in those PRs so that the tooltip body is now a div (div.bx--assistive-text), which can be a sibling or child of the tooltip trigger

the current PR just does these things: deduplicate and simplify selectors, and extract the styles for displaying the tooltip into a mixin

Nice, do we still need the &::after rule then?

yes until the next major version release (for backwards compatibility)

vpicone · 2019-09-19T14:49:58Z

packages/components/src/globals/scss/_tooltip.scss

+// Forces visible state for definition and icon only tooltip
+/// @access public
+/// @group tooltip
+@mixin tooltip--visible() {


Is this mixin meant to be public/used elsewhere? Seems unnecessary.

I'm ok with making it private, the idea was to allow the "tooltip visible" rules to be applied to the tooltip trigger wrapper when the tooltip trigger itself cannot display the tooltip properly (e.g. when the trigger is an input field/form control)

joshblack

Can we defer on read-only until there is a project that can take it on? Seems like there are a couple tooltip-related decisions that it impacts and I'd hate to introduce broader changes without giving it more thought.

asudoh · 2019-09-23T22:15:45Z

Let's limit the scope of tooltip change to introducing a CSS class for the focused state, as what this PR does, to have the read-only text input feature to go forward.

vpicone · 2019-10-10T16:54:55Z

Should we close this? Not sure if the issue it's attached to is still a concern?

asudoh · 2019-10-10T21:37:18Z

@shixiedesign @carbon-design-system/design Do we still want tooltip shown on read-only test input as its focused? Asking this question because #4015 and this PR seems for enhancing tooltip style for that purpose. Thanks!

vpicone · 2019-10-18T20:02:16Z

packages/components/src/globals/scss/_tooltip.scss

+
+  .#{$prefix}--assistive-text,
+  + .#{$prefix}--assistive-text {
+    clip: auto;


isn't clip: auto the default? Is the a reason we set it here?

vpicone

Not sure we're still pursuing this but lgtm!

joshblack · 2019-10-18T20:43:55Z

Should we go ahead and close this unless there is a designer/dev on a project team that can pick this up?

asudoh · 2019-10-18T23:15:54Z

@emyarod I think you are still pursuing this, is it correct?

emyarod · 2019-10-21T14:22:38Z

yes the remaining work is complete minus the tooltip behavior which depends on this PR

aagonzales · 2019-10-21T16:11:21Z

I don't know the history of this problem and Shixie has been off-boarded, but was tooltip added to read-only text input as a band-aid? Why was it needed in the first place?

How do I see the read-only variant? I don't see a knob for that in the story book.

joshblack · 2019-10-21T18:18:14Z

We should try and save this for when a project team can work on/address this. I think the decisions made for this have broad impact and could require alignment from design/dev/a11y which are hard when we're all working on a different project at the moment 👍

joshblack · 2019-11-06T00:46:16Z

Going to close this until we can pick things back up 👍

emyarod added 2 commits September 17, 2019 21:55

feat(tooltip): allow custom CSS classes for displaying tooltips

d59aa9f

refactor(tooltip): avoid conditional positioning on interaction

44f6d2d

emyarod added the status: ready for review 👀 label Sep 18, 2019

emyarod requested a review from a team as a code owner September 18, 2019 03:02

ghost requested review from alisonjoseph and joshblack September 18, 2019 03:02

emyarod force-pushed the 4015-support-custom-css-class-for-showing-tooltips branch from b64f389 to 44f6d2d Compare September 18, 2019 03:03

asudoh approved these changes Sep 18, 2019

View reviewed changes

packages/components/src/globals/scss/_tooltip.scss Outdated Show resolved Hide resolved

packages/components/src/globals/scss/_tooltip.scss Show resolved Hide resolved

docs(tooltip): update mixin comments

363fb0a

joshblack reviewed Sep 18, 2019

View reviewed changes

vpicone reviewed Sep 19, 2019

View reviewed changes

emyarod mentioned this pull request Sep 19, 2019

feat(text-input): add read-only variant #3054

Closed

vpicone reviewed Sep 19, 2019

View reviewed changes

emyarod requested review from vpicone and joshblack September 20, 2019 10:59

emyarod mentioned this pull request Sep 23, 2019

feat(TooltipDefinition): support different elements as tooltip trigger #3986

Closed

joshblack reviewed Sep 23, 2019

View reviewed changes

alisonjoseph removed their request for review October 8, 2019 15:15

vpicone reviewed Oct 18, 2019

View reviewed changes

vpicone approved these changes Oct 18, 2019

View reviewed changes

joshblack closed this Nov 6, 2019

emyarod deleted the 4015-support-custom-css-class-for-showing-tooltips branch August 30, 2021 19:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(tooltip): allow custom CSS classes for displaying tooltips #4016

feat(tooltip): allow custom CSS classes for displaying tooltips #4016

emyarod commented Sep 18, 2019

emyarod commented Sep 18, 2019

netlify bot commented Sep 18, 2019

netlify bot commented Sep 18, 2019

netlify bot commented Sep 18, 2019

asudoh left a comment •

edited

Loading

netlify bot commented Sep 18, 2019 •

edited

Loading

netlify bot commented Sep 18, 2019 •

edited

Loading

netlify bot commented Sep 18, 2019 •

edited

Loading

joshblack left a comment

emyarod commented Sep 19, 2019

vpicone Sep 19, 2019

emyarod Sep 19, 2019

vpicone Sep 19, 2019

emyarod Sep 19, 2019

vpicone Sep 19, 2019

emyarod Sep 19, 2019

joshblack left a comment

asudoh commented Sep 23, 2019 •

edited

Loading

vpicone commented Oct 10, 2019

asudoh commented Oct 10, 2019

vpicone Oct 18, 2019

vpicone left a comment •

edited

Loading

joshblack commented Oct 18, 2019

asudoh commented Oct 18, 2019

emyarod commented Oct 21, 2019

aagonzales commented Oct 21, 2019

joshblack commented Oct 21, 2019

joshblack commented Nov 6, 2019

feat(tooltip): allow custom CSS classes for displaying tooltips #4016

feat(tooltip): allow custom CSS classes for displaying tooltips #4016

Conversation

emyarod commented Sep 18, 2019

Changelog

Testing / Reviewing

emyarod commented Sep 18, 2019

netlify bot commented Sep 18, 2019

netlify bot commented Sep 18, 2019

netlify bot commented Sep 18, 2019

asudoh left a comment • edited Loading

Choose a reason for hiding this comment

netlify bot commented Sep 18, 2019 • edited Loading

netlify bot commented Sep 18, 2019 • edited Loading

netlify bot commented Sep 18, 2019 • edited Loading

joshblack left a comment

Choose a reason for hiding this comment

emyarod commented Sep 19, 2019

vpicone Sep 19, 2019

Choose a reason for hiding this comment

emyarod Sep 19, 2019

Choose a reason for hiding this comment

vpicone Sep 19, 2019

Choose a reason for hiding this comment

emyarod Sep 19, 2019

Choose a reason for hiding this comment

vpicone Sep 19, 2019

Choose a reason for hiding this comment

emyarod Sep 19, 2019

Choose a reason for hiding this comment

joshblack left a comment

Choose a reason for hiding this comment

asudoh commented Sep 23, 2019 • edited Loading

vpicone commented Oct 10, 2019

asudoh commented Oct 10, 2019

vpicone Oct 18, 2019

Choose a reason for hiding this comment

vpicone left a comment • edited Loading

Choose a reason for hiding this comment

joshblack commented Oct 18, 2019

asudoh commented Oct 18, 2019

emyarod commented Oct 21, 2019

aagonzales commented Oct 21, 2019

joshblack commented Oct 21, 2019

joshblack commented Nov 6, 2019

asudoh left a comment •

edited

Loading

netlify bot commented Sep 18, 2019 •

edited

Loading

netlify bot commented Sep 18, 2019 •

edited

Loading

netlify bot commented Sep 18, 2019 •

edited

Loading

asudoh commented Sep 23, 2019 •

edited

Loading

vpicone left a comment •

edited

Loading