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

Fix close button inside tags on Chrome #1574

Merged
merged 4 commits into from
Oct 27, 2022
Merged

Conversation

MewenLeHo
Copy link
Contributor

@MewenLeHo MewenLeHo commented Oct 26, 2022

Description

Missing one vendor prefix for Chrome in order to display close button correctly.
screenshot-deploy-preview-1574--boosted netlify app-2022 10 26-17_06_15

Motivation & Context

We saw with @louismaximepiton that the close button inside some tags was missing in Chrome because there was a vendor prefix missing for this browser.

⚠️ : need to fix it before release by @julien-deramond

Types of change

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

Live previews

Checklist

  • I have read the contributing guidelines
  • My code follows the code style of the project (using npm run lint)
  • My change introduces changes to the documentation
  • I have updated the documentation accordingly
  • I have added tests to cover my changes
  • All new and existing tests passed

@netlify
Copy link

netlify bot commented Oct 26, 2022

Deploy Preview for boosted ready!

Name Link
🔨 Latest commit 9a6340d
🔍 Latest deploy log https://app.netlify.com/sites/boosted/deploys/635a7988448030000948ac19
😎 Deploy Preview https://deploy-preview-1574--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 settings.

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.

LGTM 🚀 (Nice catch 😄)

@louismaximepiton
Copy link
Member

Small issue with iOS:
image

I can see this on both Safari and on Chrome.
Tags seems to introduce an overflow on x on small screens. We might add .flex-wrap to tag' containers.

@julien-deramond julien-deramond added this to Triage in v5.2.1 via automation Oct 26, 2022
@julien-deramond julien-deramond moved this from Triage to To do in v5.2.1 Oct 27, 2022
@julien-deramond
Copy link
Member

julien-deramond commented Oct 27, 2022

Small issue with iOS: image

I can see this on both Safari and on Chrome. Tags seems to introduce an overflow on x on small screens. We might add .flex-wrap to tag' containers.

Confirmed on iPhone 14/13 with Safari/Chrome. We can't ship the v5.2.1 with this issue.

scss/_tags.scss Outdated
@@ -69,6 +69,7 @@
height: 100%;
content: "";
background-color: currentcolor;
-webkit-mask-image: escape-svg(var(--#{$boosted-prefix}close-icon)); //stylelint-disable-line property-no-vendor-prefix
Copy link
Member

Choose a reason for hiding this comment

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

I'm really wondering why we need to add this. Autoprefixer should do it automatically since it does it already for other definitions of mask-image in our project. Wondering if it has something to do with CSS vars.
Let's keep this modification but I'm gonna try to find why it does that.

Copy link
Member

Choose a reason for hiding this comment

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

If the content is anything else than mask-image: var(--o-close-icon); the transformation done by Autoprefixer is OK... 🤔
As soon as the CSS var is called --o-* it doesn't work 🤔
If we change $boosted-variable-prefix for instance to uuuu- or a-, it works well 🙃

Copy link
Member

Choose a reason for hiding this comment

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

Oh I think I know where it comes from 💡
In Autoprefixer we have this kind of code mistakes: ['-khtml-', '-ms-', '-o-']. Our Boosted prefix si o-. There must be a conflict. If I change our prefix to $boosted-variable-prefix: ms- it doesn't work neither.
So I think we can keep this modification but you should add a comment in #1553 that we could remove this line when the $boosted-prefix will be removed.

Copy link
Member

Choose a reason for hiding this comment

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

@julien-deramond julien-deramond moved this from To do to In progress in v5.2.1 Oct 27, 2022
@sonarcloud
Copy link

sonarcloud bot commented Oct 27, 2022

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

@julien-deramond julien-deramond merged commit 7b3c494 into main Oct 27, 2022
@julien-deramond julien-deramond deleted the main-mlh-fix-tags-chrome branch October 27, 2022 13:27
@julien-deramond julien-deramond moved this from In progress to Done in v5.2.1 Oct 27, 2022
This was referenced Oct 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants