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

Global headers 710 #1099

Merged
merged 12 commits into from
Apr 5, 2022
Merged

Global headers 710 #1099

merged 12 commits into from
Apr 5, 2022

Conversation

louismaximepiton
Copy link
Member

@louismaximepiton louismaximepiton commented Feb 14, 2022

New branch of Global headers #727.

Closes #702 Closes #703 Closes #704 Closes #705 Closes #706 Closes #707 Closes #708 Closes #709 Closes #710

This pull request contains every global headers (Global Headers, OBS headers)

Preview:


DoD

Development

  • Should match specs (eg. either the Web UI Kit or any pattern from the WAI — or both…)
  • Docs added:
    • including the "Sass" part using scss-docs shortcode
    • in /about/overview/#custom-components if it is a new Orange custom component
    • in /getting-started/introduction/#components if it is a new Orange custom component that requires JavaScript (and Popper)
    • in /customize/overview#csps-and-embedded-svgs if it is a new Orange custom component that includes embedded SVGs in our CSS
    • in /forms/validation/?#supported-elements if it is a new Orange custom component that is a form control
    • in /forms/overview/ if it is a new Orange custom component that is a form control
  • Check (and fix) RTL version
  • Run linters
  • Run compilers
  • Tests added for JS-side
  • Run tests
  • Manually run BrowserStack test
  • Manually run Percy test
  • Cross-browser test:
    • Firefox ESR
    • Latest Edge, Chrome, Firefox, Safari
    • iOS Safari
    • Chrome & Firefox on Android
  • Clean up the branch using rebase -i
  • Commited with feat(…): … message
  • Mention it in Migration Guide (if back-from-v4): renamed variables, changes in markup requirement, etc.

Reviews

  • Code review
  • A11y review
  • Design review

@louismaximepiton louismaximepiton changed the title feat(headers): Add the Global Header component Global headers 710 Feb 14, 2022
@louismaximepiton louismaximepiton removed the request for review from julien-deramond February 14, 2022 10:44
@louismaximepiton
Copy link
Member Author

louismaximepiton commented Feb 14, 2022

This comment will be updated during the review

Sub Tasks

  • @Nurovek Don't repeat the definition of icons / actions in the HTML for the headers
  • @isabellechanclou Remove Navbar static now useless since the headers are always sticky
  • @isabellechanclou Navbar sticky must contain a supra bar, a simple header and a big body in order to be able to scroll down.
    • The header must be sticky.
    • The content of the supra bar must be in the burger menu in mobile / tablet mode
  • @louismaximepiton The elements of the supra bar and the header on the left and on the right are supposed to be aligned together. It is not the case from 768px to 1023px.
  • (Optional) Finish the implementation of the minimization in JS
    • Include it in Navbar sticky
    • Add a toggle in Navbar sticky to allow the user to test the header with or without the JS minimized animation
  • (Optional) Use our custom headers in the documentation (LMP)
  • (Optional) Handle options described in https://boosted.orange.com/docs/4.6/components/navbar-orange/#options. If not, mention it in Migration guide (sticky is done and waiting for new documentation)
  • At the end of the development phase, rewrite the documentation
  • At the end of the development phase, add information in /docs/5.1/migration/ if there is a difference of use with the v4
  • At the end of the development phase in the Navbar doc
    • Add a callout danger to explain that this page explains how to build generic navbars but also that an Orange project must use Orange navbars
    • Check that the general concepts are still working

Bugs

  • 🐛 @Nurovek Regression with the latest modifications: the supra bars icons must be 1px higher (see main branch already validated).
  • @louismaximepiton 🐛 Left/right alignments for the headers are not correct / not aligned with the supra bars
  • 🐛 On the supra bars texts, I sometimes have 25px and sometimes 26px (maybe not related to supra bars but to the font itself or line-height) (FF W10)
  • 🐛 @Nurovek Alignment broken in large viewports (FF W10)
    image
  • 🐛 @louismaximepiton Spacing between icons in large viewports seems to be broken (FF W10)
  • 🐛 The burger menu is broken in viewport md + is not well spaced (FF W10) (maybe with responsive task)
  • 🐛 Apart those with title, Headers appear weird in md breapoint (menu not in burger menu)
  • 🐛 Nav under doesn't have menu in small viewports
  • 🐛 ⚠️ @Nurovek Regression with the undisplaying of the active selector
    image
    => More details: caused by the overflow hidden (for the ellipsis to work). So, in the meantime I'm removing overflow hidden
  • 🐛 Navbar sticky example shouldn't contain an invalid content to avoid a propagation in Orange websites. Remove the content or find a ODS valid example with the designers.
  • 🐛 Not possible to have "Discover" and "Personal" active at the same time in the Navbar sticky example
  • 🐛 Nav under in small breakpoint has a double grey border-bottom
    image
  • 🐛 The removed nowrap adds some unwanted changes in the doc page. What to do ? Re-enable nowrap for the docs page ?
    image
  • 🐛 The "basket" and "my account" icons should be inside the burger menu (breakpoint xs) for global header with title(s)
  • 🐛 The spacings (padding + css) are not well applied for icons in xs + sm breakpoints
  • 🐛 The items in the burger menu (aka global menu) don't have a height of 50px
  • 🐛 Outline of bg-toggler is weird : white and margin-right 0
  • alignement à gauche des icones dans le burger menu
  • pas de selecteur active sur un burger-menu item (de xs à md inclus)
  • left-align les textes
  • 1px en trop (ou pas assez) sur la hauteur des burger-menu items (49 au lieu de 50)
  • nav-items à aligner sur la droite pour les obs headers

Others

@louismaximepiton

This comment was marked as outdated.

@Nurovek
Copy link
Contributor

Nurovek commented Feb 17, 2022

b5aee04 by @louismaximepiton:

  • Reorganizing of the moment when collapsed is applied on the collapse (burger) icon. For the moment in Bootstrap the collapsed class is applied for when the menu is collapsing, whereas we chose to apply it once the menu is fully collapsed (then it would be more cohesive with the complete semantic in collapse.js).

Need to check whether an issue needs to be opened Bootstrap-side.

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.

Design review

  • While I was doing the review one of your latest commits has broken the let alignment of the supra bars... So my feedback doesn't take that into account.

  • IMO the Orange navbar is used for the documentation, but we should wait for Handle .container-fluid to follow brand #987 to be merged because it doesn't work for some breakpoints. The content is not aligned with the menu. A simple change of container allowed to make it work!

Needs changes

  • In 480/320, the <h1> is 21.008px. Its size is defined with 1.313rem and should be 1.3125rem.
  • Headers with double line titles (from 1440 to 768) has a <h1> > <span> defined with 1.813rem and should be 1.8125rem.
  • Minimized header has a problem with the horizontal alignment: the baseline of the nav links is not aligned with the bottom of the Orange logo

Could be handled in another PR after the merge of this one

Issues should be created

  • Supra bars (1440 to 1080): we don't see the upper border of the outline because the supra bars are sticky on top of the screen. Same behavior with minimized header and its icons.

Screenshot from 2022-02-22 13-17-49

  • Supra bars (1440 to 1080): the active state orange line should be wider (1px -> 59px instead of 58px). It is probably caused by the 7.5px paddings (.5 pixels probably have an unexpected behavior). It could be fixed by using flex + gap.
  • Supra bars (1440 to 1080): badge blue circle should be at 4px from the top instead of 5px
  • Headers in 768: the order of focused items in the bar is weird (burger menu before the icons)
  • Headers in 768: the outline of the focused burger menu icon is kinda ugly

Screenshot from 2022-02-22 13-19-24

  • Headers in 768/480: icons are displayed in the bar and then also in the burger menu when the menu is opened. Need to clarify the behavior.
  • Headers in 340: technical choice to remove the icons of the bar. Maybe study to see if we need to enhance this behavior to keep only the first icon for example as described in the design specs
  • Headers in 768: when the menu is opened, the icons seem too large. Do we need to display them as in 480/320?
  • Headers in 768: we don't see the bottom of the outline when we focus the icons

Screenshot from 2022-02-22 13-20-47

  • Headers in 320: we don't see the left and the bottom of the outline when we focus the icons or the links. (LMP) maybe a solution for that is to play with --bs-nav-link-padding-x
    Screenshot from 2022-02-22 13-21-47
    Screenshot from 2022-02-22 13-21-35

scss/_navbar.scss Outdated Show resolved Hide resolved
scss/_navbar.scss Outdated Show resolved Hide resolved
scss/_navbar.scss Outdated Show resolved Hide resolved
scss/_navbar.scss Outdated Show resolved Hide resolved
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.

Haven't reviewed the documentation nor the shortcode but here are some technical comments (mostly related to Boosted mod).

site/layouts/shortcodes/orange-supra.html Outdated Show resolved Hide resolved
site/layouts/shortcodes/orange-supra.html Outdated Show resolved Hide resolved
site/layouts/shortcodes/orange-supra.html Outdated Show resolved Hide resolved
site/layouts/shortcodes/orange-global-headers.html Outdated Show resolved Hide resolved
site/layouts/shortcodes/orange-global-headers.html Outdated Show resolved Hide resolved
scss/_variables.scss Outdated Show resolved Hide resolved
scss/_variables.scss Outdated Show resolved Hide resolved
scss/_variables.scss Outdated Show resolved Hide resolved
scss/_variables.scss Show resolved Hide resolved
scss/_variables.scss Outdated Show resolved Hide resolved
@julien-deramond

This comment was marked as resolved.

//// Ensure headings variant align consistently
.title,
.two-lined {
margin: divide(-2em, 7) $spacer divide(-1em, 7) calc(var(--#{$prefix}navbar-brand-logo-size) / 2); /* stylelint-disable-line function-disallowed-list */
Copy link
Member

Choose a reason for hiding this comment

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

I observe that the title (on 1 line) is slighty off (1 or 2px). As a test in my browser if I change this line to margin: divide(-2em, 7) $spacer -4px calc(var(--#{$prefix}navbar-brand-logo-size) / 2); /* stylelint-disable-line function-disallowed-list */ it fixes the alignment (on my side) of the title on 1 line but breaks the title on 2 lines.

Copy link
Member

Choose a reason for hiding this comment

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

More info about this issue.

Ubuntu > Chrome

  • 🟢 Screenshot from 2022-04-05 12-40-21

  • 🔴 Screenshot from 2022-04-05 12-42-14

Ubuntu > Firefox

  • 🔴 Screenshot from 2022-04-05 12-45-13
  • 🔴 Screenshot from 2022-04-05 12-44-48

Mac > Firefox and Chrome

🟢 Perfectly aligned

@julien-deramond julien-deramond self-requested a review April 5, 2022 11:44
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.

Sounds good to me.

Before merging:

  • Check the RTL mode
  • Check if we can't fix the bug regarding the alignment of the titles observed in comment and try to establish on which browsers / OS the problem is with Browserstack. It can end up as a bug after the merge.

After the merge:

  • Gather all unresolved comments, check if they are still relevant and transform them into issues in the backlog as enhancements or bugs

@louismaximepiton

This comment was marked as resolved.

@louismaximepiton
Copy link
Member Author

louismaximepiton commented Apr 5, 2022

For the bug on browserstack, I couldn't spot any difference :
Iphone Chrome :
image


Iphone Safari :
image


Android Chrome :
image


Android Firefox :
image


Android internet :
image


Ipad Chrome :
image


Ipad Safari :
image


Windows 11 Opera :
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment