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

Missing overflow-visible on some icon button examples #2701

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

hannahiss
Copy link
Member

@hannahiss hannahiss commented Aug 21, 2024

Note: Please transform - [ ] into - (NA) in the description when things are not applicable

Related issues

Description

Add missing class .overflow-visible utility, like the other examples arround.

Motivation & Context

This class is needed to prevent SVG content from being cropped

Types of change

  • Bug fix (non-breaking which fixes an issue)

Live previews

Copy link

netlify bot commented Aug 21, 2024

Deploy Preview for boosted ready!

Name Link
🔨 Latest commit 3f9593d
🔍 Latest deploy log https://app.netlify.com/sites/boosted/deploys/66fc1bc4381c340008855e30
😎 Deploy Preview https://deploy-preview-2701--boosted.netlify.app
📱 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 configuration.

@hannahiss hannahiss marked this pull request as ready for review August 23, 2024 14:37
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.

Since our <svg>s have their own security margin, I'm wondering if we still need the explanation.

If you think that we should keep them:

  • 20 are missing in dark-mode.md line 1403 up to 1507 for the close buttons.
  • 30 are missing in dark-mode.md line 8356 up to 8821 for the close buttons.
  • 4 are missing in close-button.md line 43 up to 58 for the close buttons.
  • 2 are missing in tooltips.md line 141 up to 157 for the directions.
  • ;I think that the svgs inside the theme toggler should be changed and in general .nav-link/.nav-icon. If this is the case, many more are coming and I don't know the limit but maybe almost all svg should get .overflow-visible.
  • 2 are missing in download-app line 367 up to 378 for the outline buttons.
  • 1 is missing in icons.md line 182 for the outline buttons.
  • 6 are missing in checks-radios.md line 315 up to 357 for the no outline buttons.
  • 1 is missing in docs.html line 39 for the On this page button.
  • 1 is missing in single.html line 19 for the Download button.
  • 1 is missing in docs-subnav.html line 6 for the Toggle button.
  • 1 is missing in masthead.html line 23 for the Read the docs button.
  • 2 are missing in ;card.html line 41 up to 49 for the Sahre and Favorite buttons.
  • 2 are missing in example.html line 35 and 38 for the Copy and StackBlitz buttons.
  • 1 is missing in js-docs.html line 43 for the Copy button.
  • 1 is missing in scss-docs.html line 56 for the Copy button.

On the other side if you think that there are not needed anymore:

  • 40 to remove in dark-mode.md.
  • 6 to remove in buttons.md.
  • 3 to remove in orange-navbar.md.
  • 3 to remove in orange-global-headers.html.
  • 2 to remove in form.html example.
  • 2 to remove in orange-supra.html.

@hannahiss
Copy link
Member Author

Since our <svg>s have their own security margin, I'm wondering if we still need the explanation.

@julien-deramond
I did some testing: removing the .overflow-visible class completely, I couldn't reproduce any issues with SVGs being cropped, so I wonder if we could remove this from the documentation completely? What do you think?

@julien-deramond
Copy link
Member

julien-deramond commented Oct 1, 2024

@julien-deramond
I did some testing: removing the .overflow-visible class completely, I couldn't reproduce any issues with SVGs being cropped, so I wonder if we could remove this from the documentation completely? What do you think?

Let’s approach this with a logical deduction based on our current knowledge.

None of us have identified a specific reason why the .overflow-visible class is applied to Boosted's buttons with icons, nor have the history of this specificity. Similarly, we are not aware of any browser-related issues that would cause icons to be cropped in this context.

If we consider how other implementations handle this:

  • In Bootstrap's examples, there’s no .overflow-visible rule applied to the "Download examples" button containing an icon.
  • Similarly, Adobe’s Spectrum Buttons and other component libraries do not apply this rule either.
  • Our own Close button and color theme changer in the header don't have this .overflow-visible class

The only valid use case I can think of is if an icon or SVG has a very specific rendering requirement, for instance:

<button type="button" class="btn btn-primary">
  <svg width="1rem" height="1rem" fill="currentColor" aria-hidden="true" class="me-1">
    <text x="0" y="1.2rem" font-size="1rem">Text</text>
  </svg>
  Primary
</button>

However, our components are intended to be used with Solaris icons or, in the future, Orange’s pre-packaged icon libraries, where all icons are designed to fit uniformly within a square container of consistent size.

Given this, I’d estimate that in 99% of cases, overflow-visible is unnecessary.

This leads to two conclusions:

  1. By default, we can safely remove the .overflow-visible class from all buttons containing icons.
  2. In the documentation, we can maybe add a note stating that if an SVG is cropped in rare cases, the .overflow-visible class can be applied to fix the rendering. And I'm not 100% certain that it's useful. It's more like to not forget that it existed in the past 😄

What do you think folks?

@hannahiss
Copy link
Member Author

hannahiss commented Oct 1, 2024

This leads to two conclusions:

  1. By default, we can safely remove the .overflow-visible class from all buttons containing icons.
  2. In the documentation, we can maybe add a note stating that if an SVG is cropped in rare cases, the .overflow-visible class can be applied to fix the rendering. And I'm not 100% certain that it's useful. It's more like to not forget that it existed in the past 😄

What do you think folks?

I'm in 😄

@julien-deramond
Copy link
Member

@hannahiss We should probably add a note in the migration guide as the markup of some components are modified (even if it doesn't have any impacts for the users).

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.

I'm fine removing this class since it doesn't bring much.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Need Lead Dev Review
Development

Successfully merging this pull request may close these issues.

3 participants