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

Backport navbar rendering from upstream #422

Merged
merged 6 commits into from
Mar 11, 2024
Merged

Conversation

julien-deramond
Copy link
Member

@julien-deramond julien-deramond commented Mar 8, 2024

Description

This PR backports the navbar rendering from upstream:

  • src/assets/scss/_navbar.scss is basically a copy of the upstream file. The only specificity is the hr { margin: 1rem 0; } that is added in order to override the global hr rule defined in this project
  • src/layouts/partials/icons/bootstrap-white-fill.svg has been replaced by upstream version to include <title>{{ with .title }}{{ . }}{{ else }}Bootstrap{{ end }}</title>
  • src/layouts/partials/navbar.html:
    • Drop deprecated .navbar-dark class
    • Change to <div class="d-lg-none" style="width: 2rem;"></div> so that the logo is perfectly aligned with the one of the main website when switching from one to another
    • Added missing tabindex="-1" to the offcanvas
    • Didn't add data-bs-scroll="true" to the offcanvas. Waiting for Docs: drop data-bs-scroll="true" from navbar bootstrap#39766 to be merged to confirm this choice.

Live preview

https://deploy-preview-422--bootstrapblog.netlify.app/

@julien-deramond julien-deramond marked this pull request as draft March 9, 2024 08:52
@XhmikosR
Copy link
Member

I don't think we need the changes in src/_includes/icons/bootstrap-white-fill.svg. We only include the icon once in this repo.

@julien-deramond
Copy link
Member Author

I don't think we need the changes in src/_includes/icons/bootstrap-white-fill.svg. We only include the icon once in this repo.

In this repo and upstream, we use it 2 times: in the navbar and in the footer:

  1. In the navbar:
{{ partial "icons/bootstrap-white-fill.svg" (dict "class" "d-block my-1" "width" "40" "height" "32") }}
  1. In the footer:
{{ partial "icons/bootstrap-white-fill.svg" (dict "class" "d-block me-2" "width" "40" "height" "32") }}

Since we never use the title parameter, I can modify this change by letting the <title>Bootstrap</title> for a11y in the SVG but by removing the unused title parameter management (<title>{{ with .title }}{{ . }}{{ else }}Bootstrap{{ end }}</title>)

If you're OK with that change, you can thumb up this comment and I'll make the change in this PR and upstream.

@XhmikosR
Copy link
Member

XhmikosR commented Mar 10, 2024 via email

@julien-deramond
Copy link
Member Author

It only added this part to the SVG: <title>{{ with .title }}{{ . }}{{ else }}Bootstrap{{ end }}</title>

@XhmikosR XhmikosR marked this pull request as ready for review March 11, 2024 06:26
@XhmikosR XhmikosR merged commit aa643b6 into main Mar 11, 2024
8 checks passed
@XhmikosR XhmikosR deleted the main-jd-bd-navbar-refactoring branch March 11, 2024 06:26
XhmikosR added a commit that referenced this pull request Mar 11, 2024
@XhmikosR
Copy link
Member

@julien-deramond it seems the navbar logo now is black but the preview was looking fine

image

I'm going to revert the merge and we can revert the revert and fix it later.

XhmikosR added a commit that referenced this pull request Mar 11, 2024
@XhmikosR
Copy link
Member

I could have been CloudFlare caching, totally forgot to purge the cache before reverting this...

XhmikosR added a commit that referenced this pull request Mar 11, 2024
XhmikosR added a commit that referenced this pull request Mar 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants