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 align SVG icon with text in masthead notice #40897

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

Conversation

MohamadSalman11
Copy link
Contributor

@MohamadSalman11 MohamadSalman11 commented Oct 2, 2024

Description

The SVG icon (arrow right) was not aligned correctly with the masthead notice text when the screen size became smaller than 576px because the class d-sm-inline-flex is used, which removes the display: inline-flex; when the screen size is below 576px. But this needed to be removed because when the masthead notice breaks to a new line, the entire element looks weird. Therefore, I came with a slightly different solution that resolved the issue.

Before:
Screen size ≥576px
old-1
Screen size <576px
old

After:
Screen size ≥576px
chrome_bBxfRj1Zys
Screen size <576px
chrome_qv3SMLoCvP

Motivation & Context

  1. The align-items-center and gap-1 classes only worked when the d-sm-inline-flex class was active. When the screen size is smaller than 576px, d-sm-inline-flex class is removed, which leaves unused styles in the file and makes it bigger for no reason. The new solution avoids this.
  2. Keeps the arrow icon and text aligned, no matter the screen size or if the text wraps onto a new line.
  3. It's always good to try to make everything look its best on all different screens.

Type of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Refactoring (non-breaking change)
  • Breaking change (fix or feature that would change existing functionality)

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

Live previews

Related issues

@MohamadSalman11
Copy link
Contributor Author

@julien-deramond Hi Julien I see that some checks were not successful. Do I need to refactor and then commit directly in these?
Thank you!

@julien-deramond
Copy link
Member

Indeed, there's a linting issue in https://github.com/twbs/bootstrap/actions/runs/11147229051/job/30983805715?pr=40897. You can run npm run lint locally to test before pushing.
Don't worry about the "JS Tests" one, sometimes it fails, we'll relaunch it if needed.

@MohamadSalman11
Copy link
Contributor Author

Indeed, there's a linting issue in https://github.com/twbs/bootstrap/actions/runs/11147229051/job/30983805715?pr=40897. You can run npm run lint locally to test before pushing. Don't worry about the "JS Tests" one, sometimes it fails, we'll relaunch it if needed.

Thank you for your response. I have fixed the Stylelint issue. Hopefully, the linting will be successful now. 😊

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.

Small changes and I'll be fine on my side.

site/layouts/partials/home/masthead.html Outdated Show resolved Hide resolved
site/assets/scss/_masthead.scss Outdated Show resolved Hide resolved
MohamadSalman11 and others added 2 commits November 14, 2024 12:05
Co-authored-by: Louis-Maxime Piton <louismaxime.piton@orange.com>
Co-authored-by: Louis-Maxime Piton <louismaxime.piton@orange.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Review in progress
Development

Successfully merging this pull request may close these issues.

3 participants