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

Add a "Close" tooltip to close buttons #1823

Merged
merged 35 commits into from
Jun 19, 2023
Merged

Conversation

hannahiss
Copy link
Member

@hannahiss hannahiss commented Feb 7, 2023

Add tooltip on close buttons

Related issues

Closes #1773

Description

Tooltip added on close button for components:

  • Close button component
  • Modal component
  • Alerts component
  • Toasts component
  • Navbar component
  • Offcanvas component

To be noted:

Motivation & Context

Compliance to design UI kit v5

Types of change

  • New feature (non-breaking change which adds functionality)

Live previews

Checklist

Contribution

Accessibility

  • My change follows accessibility good practices; I have at least run axe

Design

  • My change respects the design guidelines defined in Orange Design System
  • My change is compatible with responsive display

Development

  • My change follows the developer guide
  • I have added JavaScript unit tests to cover my changes

Documentation

  • My change introduces changes to the documentation and/or I have updated the documentation accordingly

Checklist (for Core Team only)

  • My change introduces changes to the migration guide
  • (NA) My new component is added in Storybook
  • (NA) My new component is compatible with RTL
  • Manually run BrowserStack tests
  • Manually test browser compatibility with BrowserStack (Chrome >= 60, Firefox >= 60 (+ ESR), Edge, Safari >= 12, iOS Safari, Chrome & Firefox on Android)
  • Code review
  • Design review
  • A11y review

After the merge

…+ offcanvas.

Needed to add js to manage tooltip on live alert's btn-close
Still have to fix tooltip z-index on toasts when inside toast container
@netlify
Copy link

netlify bot commented Feb 7, 2023

Deploy Preview for boosted ready!

Name Link
🔨 Latest commit fd33bb5
🔍 Latest deploy log https://app.netlify.com/sites/boosted/deploys/649054ffc80f3600088cb8ee
😎 Deploy Preview https://deploy-preview-1823--boosted.netlify.app/docs/5.3/components/toasts
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

site/assets/js/snippets.js Outdated Show resolved Hide resolved
@hannahiss
Copy link
Member Author

hannahiss commented Feb 8, 2023

Another thing to notice:

For Toast custom content, since we don't have toast-header class, close button has the standard size and is not smaller like in other toasts. It is strange with toast border, see below:

image

This is not new in this PR, it was less noticeable before because background had a bg-light:

image

But maybe this is not a problem? I checked the callout PR: no ODS incompatibility callout seem planned?

==> issue created: #1875

- change js to select all btn-close
- add data-bs-container for some toasts to resolve z-index pbs
- change js-dismiss short code 2nd example: remove btn-close class and make it a standard button (with no tooltip)
- enhance migration guide
@hannahiss hannahiss marked this pull request as ready for review March 7, 2023 09:42
@julien-deramond
Copy link
Member

since we don't have toast-header class, close button has the standard size and is not smaller like in other toasts.

Can't you add a CSS rule to have the same rendering when it's a .toast-body as well?

@hannahiss
Copy link
Member Author

hannahiss commented Mar 7, 2023

since we don't have toast-header class, close button has the standard size and is not smaller like in other toasts.

Can't you add a CSS rule to have the same rendering when it's a .toast-body as well?

Actually, it should be on .toast and divergent from Bootstrap, so not so simple...
I created an issue: #1875

Copy link
Member

@louismaximepiton louismaximepiton left a comment

Choose a reason for hiding this comment

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

Some few changes before Seal of Approve !

site/assets/js/snippets.js Outdated Show resolved Hide resolved
site/assets/js/snippets.js Outdated Show resolved Hide resolved
site/content/docs/5.3/migration.md Outdated Show resolved Hide resolved
Copy link
Member

@louismaximepiton louismaximepiton left a comment

Choose a reason for hiding this comment

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

Forgot about one thing

site/content/docs/5.3/components/alerts.md Outdated Show resolved Hide resolved
Copy link
Member

@julien-deramond julien-deramond left a comment

Choose a reason for hiding this comment

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

  1. Now that our close button is always shown associated with a tooltip, it might be worth adding it to the list of "Show components requiring JavaScript" (in /docs/5.3/getting-started/introduction/#js-components).

  2. Our cookies management panel uses .btn-close. IDK if you can add the tooltip as well. Not mandatory IMO if too tricky but it may be worth a try.

Screenshot 2023-06-08 at 08 38 39
  1. IMO, /docs/5.3/components/alerts/#dismissing should be enhanced with an explanation of the tooltip which needs extra data-bs code + loading Boosted Tooltip JS.
  • Check the same thing for the other components (Close button description itself, modals, offcanvases, etc.) to see if the descriptions must be enhanced.

site/assets/js/snippets.js Show resolved Hide resolved
site/assets/js/snippets.js Outdated Show resolved Hide resolved
site/content/docs/5.3/migration.md Outdated Show resolved Hide resolved
@hannahiss
Copy link
Member Author

  1. Now that our close button is always shown associated with a tooltip, it might be worth adding it to the list of "Show components requiring JavaScript" (in /docs/5.3/getting-started/introduction/#js-components).

OK, I add "Close button" component in the list. Do I need to precise in other components that JS is used for close button? (all these components alredy use JS: Modal, Alerts, Toasts, Navbar, Offcanvas)

@julien-deramond
Copy link
Member

OK, I add "Close button" component in the list. Do I need to precise in other components that JS is used for close button? (all these components alredy use JS: Modal, Alerts, Toasts, Navbar, Offcanvas)

It's an indirect dependency so I'm not sure how to handle it TBH.

@hannahiss hannahiss self-assigned this Jun 9, 2023
@hannahiss
Copy link
Member Author

OK, I add "Close button" component in the list. Do I need to precise in other components that JS is used for close button? (all these components alredy use JS: Modal, Alerts, Toasts, Navbar, Offcanvas)

It's an indirect dependency so I'm not sure how to handle it TBH.

I tried something by adding some info on tooltip enabling on each component, at different places depending on the context. I add some info in Intro > JS components, and in migration guide.

I also tried to add tooltip on .btn-close of the cookies management modal, but there was an issue: tooltip sometimes appears before hovering... (not always)

Yu tell me what you think :-)

Copy link
Member

@julien-deramond julien-deramond left a comment

Choose a reason for hiding this comment

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

I'm down with your suggestion of messages throughout the documentation. Let's see if folks understand it easily, we can still modify them later.
Would be nice to check if this section could be a reusable shortcode.
Regarding the cookies management panel, let's create a new issue to tackle it later.

@sonarcloud
Copy link

sonarcloud bot commented Jun 19, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

Add a "Close" tooltip to close buttons
5 participants