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): add definition tooltip #714

Merged
merged 24 commits into from
May 22, 2018
Merged

feat(tooltip): add definition tooltip #714

merged 24 commits into from
May 22, 2018

Conversation

aledavila
Copy link
Contributor

@aledavila aledavila commented Apr 19, 2018

Overview

https://github.ibm.com/carbon/issues/issues/618
https://github.ibm.com/carbon/issues/issues/549#issuecomment-4988198
#771

Added

Added new tooltips with on click tooltip being the default.
Added definition tooltip
Added icon tooltip

Removed

Removed on hover tooltip.
Removed simple tooltip

Changed

Simple tooltip is just for defining the icon.
Default font weight is now normal. Bold is a modifier.

Notes

Tayler needs to sign of and look at styles.

@alisonjoseph
Copy link
Member

@aledavila These don't look like they are related to this PR? #618 #549

@aledavila
Copy link
Contributor Author

@alisonjoseph yes you are right sorry. those are the issue #s in IBM carbon repo no this repo

@aledavila
Copy link
Contributor Author

Fixed the links

Copy link
Member

@alisonjoseph alisonjoseph left a comment

Choose a reason for hiding this comment

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

Overall looks good 😄

Looks like the triangle needs to move 1px down? Screen shot of a screen shot I zoomed in on.
screen shot 2018-04-19 at 12 40 28 pm

Changing the functionality of the existing tooltip would be a breaking change. (from hover to click). Would maybe be better to add it as a new variant or v2, thoughts?

max-width: rem(240px);
padding: $spacing-lg;
max-width: rem(170px);
padding: rem(10px) $spacing-xs;
Copy link
Member

Choose a reason for hiding this comment

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

Might be a question for design but can rem(10px) be tweaked up or down to fit into our spacing scale?

Copy link
Member

Choose a reason for hiding this comment

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

It should be 8px

@aledavila
Copy link
Contributor Author

Thanks I’ll look into the changes. As it stands how the changes were made it doesn’t break anything rather people who have the tooltip implemented would have to click on it now instead of hovering when they pull the new changes. But i don’t know how you guys prefer to handle that.

Yea I’ll ask Tayler about the changes probably better to have the padding all spacing-xs.

@asudoh
Copy link
Contributor

asudoh commented Apr 19, 2018

Agreed with @alisonjoseph that looks great overall 🎉 - Thanks @aledavila!

The only question I have is how to handle the change in behavior, which would need to involve designers (CC @carbon-design-system/design) - Should we introduce some way to keep the older behavior, or should we try something to ensure that the change in behavior does not come with a surprise to our consuming services...?

@aledavila
Copy link
Contributor Author

@alisonjoseph I changed the 1 pixel that was off. I'll ask Tayler about the other padding issue when she gets back.

@aagonzales
Copy link
Member

Changing the functionality of the existing tooltip would be a breaking change. (from hover to click). Would maybe be better to add it as a new variant or v2, thoughts?

I think making it a v2 and deprecating the v1 version with Carbon V10 would be fine. Consumers just won't ever automatically get those update, correct? They'll have to manually update it to v2?

@alisonjoseph
Copy link
Member

correct @aagonzales. That's how we've been handling other components with breaking changes.

@aledavila
Copy link
Contributor Author

Oh I see. Can someone explain how that process is or where to make it a V2 instead?

@alisonjoseph
Copy link
Member

Looks like the process for the other components is just updating the classname, so bx--tooltip-v2 and just make sure that the original tooltip doesn't change. You can see how the v2 input styles are handled in this PR for an example. #712

@aledavila
Copy link
Contributor Author

@alisonjoseph I added the v2 now. I just need to ask Tayler about the padding on the simple tooltip. Everything else should be good.

@@ -0,0 +1,8 @@
<div class="bx--tooltip--simple-v2">
Copy link
Member

@alisonjoseph alisonjoseph Apr 25, 2018

Choose a reason for hiding this comment

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

Should this class be bx--tooltip-v2--simple?

Copy link
Contributor

@asudoh asudoh left a comment

Choose a reason for hiding this comment

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

Hi @aledavila great to see you complete V2 split!

@@ -0,0 +1,25 @@
### HTML
Copy link
Contributor

Choose a reason for hiding this comment

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

Given V2 is newly introduced component, does this migration guide still make sense? One thing that comes to my mind is V1 to V2 migration guide. Another approach may be simply keeping the README in-sync with what V2 does.

@tay-aitken
Copy link
Contributor

@aledavila can you create a staging link so that I can inspect the element and see how everything looks. Thanks! This will help me make a decision about the padding.

@tay-aitken
Copy link
Contributor

tay-aitken commented Apr 27, 2018

Fixes

Please take a a look at the specs I posted previously to make sure the coded variation matches the designs and make the following changes below:

  • Change default tooltip name to Interactive
  • Change simple name to Icon

Interactive tooltip

  • Using the incorrect icon. Let me know if you need an SVG of the right one/if it needs to be added to our icon list. The SVG can be found here
  • add in a link and button as seen here
  • Tooltip should not close if i click inside the tooltip, should only close if I click outside of the tooltip area
  • background is not black, it should be background-color: #272D33; (and should be using our color tokens)
  • Tooltip icon should be darkening on hover by 10% not lightening (sync with TJ, his PR might already be taking care of this)
  • Tooltip label should not be changing color on hover, should remain #152935
  • Can't inspect the tooltip box itself, having trouble locating it in the code, will review colors, type sizes, padding, etc. once link and button are added in

Definition Tooltip

  • Please make the tooltip have the following content "Brief description of the dotted, underlined word above."
  • Is there a reason the old CSS for the tooltip still is a part of this component?
  • Padding on all sides should be .05rem
  • Dotted underline should be #8897A2 (ui-04) on enabled
  • on :hover it should be #3d70b2 (brand-01)
  • should be font-size: 0.75rem; not 0.875rem
  • Tooltip icon should be darkening on hover by 10% not lightening (sync with TJ, his PR might already be taking care of this)
  • Double check that the tooltip is only 170px wide, perhaps add in a max-width
  • Change label to Definition label, instead of Tooltip label

Icon Tooltip

  • Should be background-color: #272D33;, not white
  • Should be border: none;
  • Remove the label
  • Use Filter icon instead of the tooltip icon
  • Tooltip content should say "Filter"
  • Tooltip is not aligned with top of icon
  • Padding should be 4px (.025rem) on all sides
  • Should be font-size: 0.75rem; (12px)
  • Make sure the shark fin is behind the tooltip background
  • For example purposes please have the tooltip appear below the icon, not above.

@aledavila
Copy link
Contributor Author

aledavila commented May 4, 2018

@alisonjoseph

Ignore the colors. They will be updated once TJs updated colors are merged. The CSS tooltips have the right color but as a hex for the time being. Renaming Default Tooltip to Interactive Tooltip is more complex than simply changing file name. Possible workaround could be name change in website itself.

Definition: I changed the definition tooltip completely and took made it a separate element instead of creating it with a before. It helped with being able to give it a max width and if more than the max widt
h it will wrap appropriately. And re did the CSS for it.

Default: As for the JS I added the prevent close when clicking inside the tooltip. Only close when clicking outside.

Simple --> Icon: Simple tooltip is now Icon only tooltip.

Copy link
Member

@alisonjoseph alisonjoseph left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

Copy link
Contributor

@asudoh asudoh left a comment

Choose a reason for hiding this comment

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

LGTM basically - Great to see this PR getting there @aledavila! Just one little thing - Is it possible for you to remove yarn-error.log and package-lock.json from the source control before merging? Thanks!

@aledavila aledavila changed the title [WIP] feat(tooltip): add definition tooltip DO NOT MERGE feat(tooltip): add definition tooltip DO NOT MERGE May 15, 2018
@aledavila aledavila changed the title feat(tooltip): add definition tooltip DO NOT MERGE feat(tooltip): add definition tooltip May 15, 2018
@alisonjoseph
Copy link
Member

@aledavila since the demo template changes have been merged in to the v9 branch, would be nice to update this from html to handlebar templates before we merge it in.

@asudoh
Copy link
Contributor

asudoh commented May 18, 2018

^ Yes it'll be nice - Simple rename from .html to .hbs will work for now, but great to merge several component variant's markup using the power of Handlebars. Feel free to ping me if you have any questions - Thanks!

Copy link
Member

@alisonjoseph alisonjoseph left a comment

Choose a reason for hiding this comment

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

Would be good to add correct aria roles. Ref #771

https://www.w3.org/TR/wai-aria-practices/#tooltip

WAI-ARIA Roles, States, and Properties
The element that serves as the tooltip container has role tooltip.
The element that triggers the tooltip references the tooltip element with aria-describedby.

Copy link
Member

@tw15egan tw15egan left a comment

Choose a reason for hiding this comment

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

Just a few tiny changes, but other than that it looks great!

border: 1px solid $ui-03;
border-radius: 4px;
Copy link
Member

Choose a reason for hiding this comment

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

Can we switch this to rem(4px)?

margin-top: $spacing-sm;
padding: $spacing-xs;
color: $inverse-01;
border-radius: 4px;
Copy link
Member

Choose a reason for hiding this comment

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

Can we switch this to rem(4px)?

left: 0;
width: 0.6rem;
height: 0.6rem;
margin-left: 24px;
Copy link
Member

Choose a reason for hiding this comment

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

Can we switch this to rem(24px)?

margin-left: 50%;
color: $text-01;
padding: $spacing-2xs;
border-radius: 4px;
Copy link
Member

Choose a reason for hiding this comment

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

Same as above

@aledavila
Copy link
Contributor Author

WIP: working on accessibility

<path d="M8 0C3.6 0 0 3.6 0 8s3.6 8 8 8 8-3.6 8-8-3.6-8-8-8zm0 4c.6 0 1 .4 1 1s-.4 1-1 1-1-.4-1-1 .4-1 1-1zm2 8H6v-1h1V8H6V7h3v4h1v1z"></path>
</svg>
</a>
<div class="bx--tooltip__label">
Copy link
Member

Choose a reason for hiding this comment

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

We should add the aria-describedby here <div class="bx--tooltip__label" aria-describedby="unique-tooltip">

</a>
<div class="bx--tooltip__label">
Tooltip Label
<div tabindex="0" data-tooltip-trigger data-tooltip-target="#unique-tooltip" role="tooltip" aria-labelledby="unique-tooltip" class="bx--tooltip__trigger">
Copy link
Member

Choose a reason for hiding this comment

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

you can remove aria-labelledby="unique-tooltip" here

Copy link
Member

@alisonjoseph alisonjoseph left a comment

Choose a reason for hiding this comment

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

Looks good! 🎉

@alisonjoseph alisonjoseph merged commit a22c156 into carbon-design-system:v9 May 22, 2018
asudoh pushed a commit that referenced this pull request May 30, 2018
* feat(tooltip): add definition tooltip

* feat(tooltip): add on click tooltip

* chore(tooltip): simple info tooltip

* fix(tooltip): fix arrowhead 1 pixel

* chore(tooltip): add v2 of tooltip in seperate folder

* chore(tooltip): add previous version of tooltip

* chore(tooltip): add files to globals

* fix(tooltip): fix simple tooltip class name

* chore(tooltip): add definition tooltip to readme

* chore(tooltip): add tooltip icon

* chore(tooltip): change definition tooltip to seperate element

* fix(tooltip): fix merge conflict again

* chore(tooltip): removed v2 and added new color variables

* chore(tooltip): add bold modifier

* chore(tooltip): updated migration to v9 and readme files

* chore(tooltip): add focus to definition

* chore(tooltip): remove tab

* fix(tooltip): added accessibility aria tags

* chore(tooltip): change arrow to be a dom element

* chore(tooltip): change aria label location

* chore(tooltip): attribute name change
joshblack pushed a commit that referenced this pull request Jun 4, 2018
* feat(inputs): fixed old pr and added inputs and colors work (#756)

* feat(inputs): fixed old pr and added inputs and colors work

* feat(inputs): fixed inline select

* chore(devenv): introduced parameterized demo HTML (#579)

* feat(code-snippet): Add new Code snippet variation and styles (#761)

* feat(code-snippet): Update code snippet styles and names

* feat(code-snippet): Update font size

* chore: cleanup html files

* feat(code-snippet): Update feedback copy tooltip to match new styles

* chore: cleanup javascript

* feat(code-snippet): Update copy tooltip styles

* chore: cleanup js

* feat(code-snippet): Update click event to only happen on button

* chore: Add back support for deprecated class names

* feat(code-snippet): Update tooltip styles and positioning

* fix: update tooltip positioning for inline snippet

* V9 merge 05162018 (#773)

* fix(data-table-v2): fix the icon that becomes small when column header wraps, resolves #720 (#742)

* fix(tooltip): Firefox overflowing text when you have a long enough string without spaces (#737)

* feat(dropdown): added inline dropdown, style changes to dropdown, fixed link in data table readme  (#746)

* feat(dropdown): added inline dropdown

* feat(dropdown): fixed js

* docs(components): add component model explainer (#739)

* fix(grid): center grid at largest breakpoint (#747)

* fix(letter-spacing): Remove letter-spacing (#748)

* fix(letter-spacing): remove letter-spacing across the board

* fix(letter-spacing): change hard-coded letter-spacing values to use the mixin

* fix(select): added disabled styling to option in select (#751)

* fix(select): added disabled styling to select item

* fix(select): removed disabled

* fix(select): added cursor not allowed

* fix(dropdown): added tabindex logic to dropdown (#749)

* fix(input): remove red focus ring when input marked as required firefox (#750)

* fix(multi-select): fixed width and hover color (#752)

* feat(Skeleton-states): Add skeleton state styles (#713)

* feat(skeleton): Add skeleton loading mixin and styles

* feat(skeleton): Add skeleton icon styles

* feat(skeleton): Add skeleton styles to components

* feat(skeleton): Add structured list skeleton

* feat: skeleton styles

* feat: add skeleton demo html

* feat: update skeleton html

* feat(skeleton-states): Update accordion skeleton styles

* feat(skeleton-states): Update accordion

* chore: Update px to rem

* chore: remove duplicate code

* chore: Add correct color value for skeleton state

* feat(skeleton-states): Update data table

* fix(skeleton-states): Add helper-mixins import to components using skeleton states (#755)

* fix(data-table): update data-table to use the new search component (#754)

* fix(structured-list): Update header alignment to bottom to match designs (#753)

* feat(truncation): add truncation classes (#757)

* feat(truncation): add truncation classes

* fix(truncation): update truncation classes to be more semantic

* fix(truncation): use prefix variable

* fix(structured-list): fixed column header text size (#766)

* fix(structured-list): fixed column header text size

* chore(structured-list): deleted extra file

* fix(forms): Add helper-mixins import (#767)

* fix(helper-classes): import vars file (#768)

* fix(code-snippet): Update templates (#779)

* fix(code-snippet): Update skeleton state styles

* fix: swap code and pre tag nesting order

* fix(code-snippet): Update hbs template for code snippet

* fix: spelling error

* chore: remove old files

* chore: move changes from PR 774 to this

* fix: Update click event to run on button

* fix(pagination): bring v2 version back (#782)

* chore(devenv): fix full-render demo (#783)

* V9 merge 05182018 (#785)

* feat(dropdown): added inline dropdown, style changes to dropdown, fixed link in data table readme  (#746)

* feat(dropdown): added inline dropdown

* feat(dropdown): fixed js

* fix(letter-spacing): Remove letter-spacing (#748)

* fix(letter-spacing): remove letter-spacing across the board

* fix(letter-spacing): change hard-coded letter-spacing values to use the mixin

* fix(dropdown): added tabindex logic to dropdown (#749)

* feat(Skeleton-states): Add skeleton state styles (#713)

* feat(skeleton): Add skeleton loading mixin and styles

* feat(skeleton): Add skeleton icon styles

* feat(skeleton): Add skeleton styles to components

* feat(skeleton): Add structured list skeleton

* feat: skeleton styles

* feat: add skeleton demo html

* feat: update skeleton html

* feat(skeleton-states): Update accordion skeleton styles

* feat(skeleton-states): Update accordion

* chore: Update px to rem

* chore: remove duplicate code

* chore: Add correct color value for skeleton state

* feat(skeleton-states): Update data table

* fix(font-face): bring missing $css--font-face reference back (#778)

Fixes #777.

* chore(devenv): update variant labels (#784)

* chore(devenv): update variant labels

* chore(devenv): modal config update

* feat(tooltip): add definition tooltip (#714)

* feat(tooltip): add definition tooltip

* feat(tooltip): add on click tooltip

* chore(tooltip): simple info tooltip

* fix(tooltip): fix arrowhead 1 pixel

* chore(tooltip): add v2 of tooltip in seperate folder

* chore(tooltip): add previous version of tooltip

* chore(tooltip): add files to globals

* fix(tooltip): fix simple tooltip class name

* chore(tooltip): add definition tooltip to readme

* chore(tooltip): add tooltip icon

* chore(tooltip): change definition tooltip to seperate element

* fix(tooltip): fix merge conflict again

* chore(tooltip): removed v2 and added new color variables

* chore(tooltip): add bold modifier

* chore(tooltip): updated migration to v9 and readme files

* chore(tooltip): add focus to definition

* chore(tooltip): remove tab

* fix(tooltip): added accessibility aria tags

* chore(tooltip): change arrow to be a dom element

* chore(tooltip): change aria label location

* chore(tooltip): attribute name change

* chore(files): remove yarn-error.log (#800)

* chore(package): remove npm5 lockfile and add lockfile to gitignore (#803)

* V9 merge 05212018 (#801)

* feat(dropdown): added inline dropdown, style changes to dropdown, fixed link in data table readme  (#746)

* feat(dropdown): added inline dropdown

* feat(dropdown): fixed js

* feat(Skeleton-states): Add skeleton state styles (#713)

* feat(skeleton): Add skeleton loading mixin and styles

* feat(skeleton): Add skeleton icon styles

* feat(skeleton): Add skeleton styles to components

* feat(skeleton): Add structured list skeleton

* feat: skeleton styles

* feat: add skeleton demo html

* feat: update skeleton html

* feat(skeleton-states): Update accordion skeleton styles

* feat(skeleton-states): Update accordion

* chore: Update px to rem

* chore: remove duplicate code

* chore: Add correct color value for skeleton state

* feat(skeleton-states): Update data table

* fix(tile): add check to make sure element exists (#786)

* feat(data-table-v2): add inline edit styles for cell (#780)

* fix(table): Missing border in firefox (#787)

* fix(overflow-menu): added different styles for focus and hover (#788)

* fix(overflow-menu): added different styles for focus and hover

* fix(overflow-menu): fixed danger hover/focus

* fix(data-table-v2): remove left positioning from sort icon (#789)

* fix(tile): check if is--expanded is set when tile component is loaded (#791)

* fix(danger-button): Add correct hover color for icon (#794)

* fix(table): removes selected class from row on cancel click (#793)

* chore(git): update gitignore for *.log files (#798)

* fix(button): bring back primary danger button

* chore(package): update lockfile to remove artifactory references (#808)

* Light dropdown styles (#809)

* feat(dropdown): Add light variation

* feat(list-box): Add light variation

* Icon updates (#810)

* feat(icons): start updating icons

* feat(icons): update more icons

* feat(icons): continue icon work

* feat(icons): finish up icon updates

* fix(overflow): fix hover colors on overflow menu

* fix(components): some small fixes for v9 (#812)

* 732 search (#811)

* chore(search): remove icons from search

* fix(search): fix default close button offset

* fix(form): fixed button width (#819)

* fix(docs): Update Component docs (#815)

* chore: Remove button docs ref glyphs

* fix(docs): Update component docs

* fix(code-snippet): fixed a render issue for the light version (#817)

* feat(slider): Light variation (#818)

* feat(slider): Add light variation

* fix: Fix spelling error in all config files

* feat(checkbox): Update checkboxes to match new icon set (#822)

* feat(checkbox): Update checkboxes to match new icon set

* feat: Update checkbox styles

* fix: Add code snippet example skeleton state html (#827)

* chore(tooltip): change to lowercase 'label' (#836)

* fix(search): close button positioning for custom react search (#835)

* fix(radio): Update radio + checkbox border colors (#834)

* fix(radio-button): update border color

* fix(checkbox): update checkbox colors

* fix(icons): Update data table v2 & card icons (#826)

* fix(icons): Update data table v2 & card icons

* fix: Fix toolbar search icon from being cut off

* fix(pagination): update colors (#832)

* fix(multi-select): update hover color (#830)

* fix(v9): small fixes to components (#838)

* fix(v9): small fixes to components

* fix(tooltip): align focus on icon with text

* fix(multi-select): fix HTML structure of multi select (#840)

* fix(pagination): fix text input position in pagination V1 (#841)

* fix(tooltip): Definition tooltip positioning fix (#843)

* fix(tooltip): update tooltip definition markup

* fix: Add back in js positioning

* chore: cleanup js comments

* fix(tooltips: Tooltip a11y (#846)

* fix(tooltip): update tooltip definition markup

* fix(tooltip): top positioning

* fix: A11y fixes

* chore: update html/js/readme

* chore: revert js changes.

* fix(tooltip): position fix (#848)

* fix(icons): update icons, other small fixes (#849)

* fix(icons): update icons, other small fixes

* fix(icon): add a w

* fix(code-snippet): fixed feedback not showing up in firefox (#852)

* chore: update dev env to work in IE11 (#853)

* chore: remove deprecated components (#855)

* fix(components): misc fixes for v9 (#854)

* fix(inline-dropdown): removed box-shadow

* fix(time-picker): fixed arrow being positioned weird in firefox

* fix(code-snippet): banner showing up on ie11

* fix(form): remove margins from bx--form-item (#861)

* fix(list-box): removed styles on disabled item (#862)

* fix(select): position arrow based on bottom rather than top (#864)

BREAKING CHANGE: update to v9
joshblack pushed a commit that referenced this pull request Jun 4, 2018
* feat(tooltip): add definition tooltip

* feat(tooltip): add on click tooltip

* chore(tooltip): simple info tooltip

* fix(tooltip): fix arrowhead 1 pixel

* chore(tooltip): add v2 of tooltip in seperate folder

* chore(tooltip): add previous version of tooltip

* chore(tooltip): add files to globals

* fix(tooltip): fix simple tooltip class name

* chore(tooltip): add definition tooltip to readme

* chore(tooltip): add tooltip icon

* chore(tooltip): change definition tooltip to seperate element

* fix(tooltip): fix merge conflict again

* chore(tooltip): removed v2 and added new color variables

* chore(tooltip): add bold modifier

* chore(tooltip): updated migration to v9 and readme files

* chore(tooltip): add focus to definition

* chore(tooltip): remove tab

* fix(tooltip): added accessibility aria tags

* chore(tooltip): change arrow to be a dom element

* chore(tooltip): change aria label location

* chore(tooltip): attribute name change
joshblack pushed a commit to joshblack/carbon that referenced this pull request May 2, 2019
…system#714)

* chore(package): update rollup to version 0.57.0

* chore(package): update lockfile

https://npm.im/greenkeeper-lockfile
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.

7 participants