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

Supra Bars (Standard, With Langs, With Actions) #795

Merged
merged 52 commits into from
Dec 1, 2021

Conversation

Nurovek
Copy link
Contributor

@Nurovek Nurovek commented Sep 10, 2021

Closes #699 #700 #701

Current branch for all things Supra Bar

  • Missing "Orange navbar" documentation as in v4
    Easy to forget: it must be accompagnied by a reference in About > Overview > Custom Components
  • .supra class is declared within scss/_navbar.scss. In v4, it was in another file _o-navbar.scss. Since it is a new component, I suppose we could keep it in another file called _o-navbar.scss or _orange-navbar.scss.
  • There is an unused .header-minimised class in scss/_navbar.scss.
  • Is there no hover state for the navbar contact image?
  • The responsive part seems to match the design specifications apart from the left margin of "Personal"; the margin seems too big as you will see in the following comparison between this version, v4 and the design specifications for the 3 breaking points
  • Supra Bars (Standard, With Langs, With Actions) #795 (comment)
  • Supra Bars (Standard, With Langs, With Actions) #795 (comment)

@Nurovek Nurovek changed the title Feature/supra bars Supra Bars (Standard, With Langs, With Actions) Sep 14, 2021
@Nurovek Nurovek added the v5 label Sep 15, 2021
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.

  • Missing "Orange navbar" documentation as in v4
  • .supra class is declared within scss/_navbar.scss. In v4, it was in another file _o-navbar.scss. Since it is a new component, I suppose we could keep it in another file called _o-navbar.scss or _orange-navbar.scss.
  • There is an unused .header-minimised class in scss/_navbar.scss.
  • Is there no hover state for the navbar contact image?
  • The responsive part seems to match the design specifications apart from the left margin of "Personal"; the margin seems too big as you will see in the following comparison between this version, v4 and the design specifications for the 3 breaking points:

Capture d’écran 2021-09-17 à 08 07 30

Capture d’écran 2021-09-17 à 08 07 58

Capture d’écran 2021-09-17 à 08 09 10

site/content/docs/5.1/guidelines/global-headers.md Outdated Show resolved Hide resolved
scss/_variables.scss Outdated Show resolved Hide resolved
scss/_navbar.scss Outdated Show resolved Hide resolved
@Nurovek Nurovek force-pushed the feature/supra-bars branch 4 times, most recently from b6b1f82 to ef02548 Compare September 23, 2021 14:33
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 haven't finished the review but here are some comments:

site/content/docs/5.1/about/overview.md Show resolved Hide resolved
site/data/sidebar.yml Outdated Show resolved Hide resolved
scss/_variables.scss Outdated Show resolved Hide resolved
scss/_orange-navbar.scss Outdated Show resolved Hide resolved
@julien-deramond
Copy link
Member

julien-deramond commented Oct 1, 2021

It was easier for me so I have done an active review of this PR via 7ab0658.
If you disagree with the content, you still can revert it of course.
Among the changes:

  • Fixed the rendering of icons and badges
  • Fixed the left and right breakpoints
  • Fixed the space between elements
  • Reused more v4 code instead of creating a new one
  • Added a shortcode ({{< orange-supra >}}) to only have to fill the right part of the supra bar in examples
  • Replaced temporarily the code of https://deploy-preview-795--boosted.netlify.app/docs/5.1/examples/headers/ to facilitate the tests and reviews (we must think at the end to remove this modification)

@Nurovek I let you check those modifications.
Here are some things not taken into account:

  • Badge height is 20px but should be 19px; check if it is possible.
  • Focus; something to do?
  • Check the differences (less important) between v4 and v5 to complete the migration part

@Nurovek
Copy link
Contributor Author

Nurovek commented Oct 5, 2021

  • @julien-deramond Shortcode is a great idea. At long last it will factorise a bit
  • I'm ok with reusing v4 code if it doesn't clash with v5 style of coding, but it doesn't seem to 👍

@julien-deramond
Copy link
Member

@CyriaqueBillard or @Franco-Riccitelli, here is an updated version of the supra bars:

Could you please make a design review? 🙏

@julien-deramond
Copy link
Member

julien-deramond commented Oct 6, 2021

Design review by @Franco-Riccitelli:

"The Supra bars looks very good!
I just wanted to let you know of two more small points. Thanks.

  1. The Link labels on the Supra bar (Personal, Business, EN, FR, ES) need to be moved down by 1px.
  2. The spacing between the Search, Basket and Avatar icons should be 30px.
    Please note, all the icons in the Solaris library are set within a square 'Cell'.
    This keeps the sizes of the icons consistent, as each icon has a different shape and therefore a different width and height.
    When we specify the size and positioning of the icons in Sketch, we do this against the icon Cell, not the actual shape of the icon.
    You can see this in Zeplin or ZeroHeight (in the inspector mode) when you hover your cursor over an icon."

Icon-Sizes

@Nurovek Nurovek force-pushed the feature/supra-bars branch 2 times, most recently from ed92391 to 217281d Compare October 7, 2021 08:25
@julien-deramond
Copy link
Member

julien-deramond commented Oct 28, 2021

@Nurovek When you will resolve the conflicts, don't forget to apply role="search" wherever it is needed (see 4f42dd5) and to reintegrate the content of site/content/docs/5.1/examples/headers/index.html modified for the design review.

@Franco-Riccitelli
Copy link

@Nurovek and @julien-deramond, I have completed the visual design review of the updated Supra Bar and the changes all look good.

@julien-deramond
Copy link
Member

Minor changes since my last review. @Lausselloic I let you check that the last changes are OK for you.

Copy link
Member

@Lausselloic Lausselloic left a comment

Choose a reason for hiding this comment

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

need to improve variable signification.
And take a look to the line height for small font size

scss/_orange-navbar.scss Outdated Show resolved Hide resolved
site/layouts/shortcodes/orange-supra.html Outdated Show resolved Hide resolved
scss/_variables.scss Outdated Show resolved Hide resolved
scss/_orange-navbar.scss Outdated Show resolved Hide resolved
scss/_orange-navbar.scss Outdated Show resolved Hide resolved
scss/_orange-navbar.scss Outdated Show resolved Hide resolved
scss/_orange-navbar.scss Outdated Show resolved Hide resolved
scss/_orange-navbar.scss Outdated Show resolved Hide resolved
@Nurovek
Copy link
Contributor Author

Nurovek commented Nov 25, 2021

@Lausselloic if you have some spare time to check if everything is as good as you'd intended :)

@julien-deramond
Copy link
Member

@Nurovek I've reviewed your last modifications regarding to @Lausselloic feedback. Double checked the rendering which is OK. 1e86830 is my proposal for the naming of the variables (use the same prefix for this component $navbar-orange-supra). I let you check. If you're OK with those modifications, I'll merge this PR.

@Nurovek
Copy link
Contributor Author

Nurovek commented Dec 1, 2021

@Nurovek I've reviewed your last modifications regarding to @Lausselloic feedback. Double checked the rendering which is OK. 1e86830 is my proposal for the naming of the variables (use the same prefix for this component $navbar-orange-supra). I let you check. If you're OK with those modifications, I'll merge this PR.

Fine with your tunings 👍

@julien-deramond julien-deramond merged commit 4b4666c into main Dec 1, 2021
@julien-deramond julien-deramond deleted the feature/supra-bars branch December 1, 2021 11:52
@julien-deramond julien-deramond mentioned this pull request Dec 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants