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

Footer component #970

Merged
merged 31 commits into from
Apr 22, 2022
Merged

Footer component #970

merged 31 commits into from
Apr 22, 2022

Conversation

louismaximepiton
Copy link
Member

@louismaximepiton louismaximepiton commented Dec 13, 2021

Previews :
https://deploy-preview-970--boosted.netlify.app/docs/5.1/examples/footers/
https://deploy-preview-970--boosted.netlify.app/docs/5.1/components/footer/

Closes #892

Note : Issue with Follow us get bigger on lg viewport (not on md)

First review from Julien :

  • Check where to insert Orange footer references.
  • Check for changes in Migration page.
  • Do the 2 thumbnails.
  • Check for the baseline (1px shift in WEB-FTR-STD-005).
  • Check with accessibility for more padding when outline with links (for WEB-FTR-STD-005 only ?).

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

@julien-deramond julien-deramond marked this pull request as draft December 16, 2021 09:02
@louismaximepiton louismaximepiton marked this pull request as ready for review December 16, 2021 16:46
@julien-deramond julien-deramond marked this pull request as draft December 21, 2021 14:37
@louismaximepiton louismaximepiton changed the title fix(footerExamples): Responsiveness + paddings feat(footer): Adding footer component Dec 28, 2021
@julien-deramond julien-deramond changed the title feat(footer): Adding footer component Footer component Dec 31, 2021
@julien-deramond julien-deramond marked this pull request as ready for review December 31, 2021 10:27
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.

1st serie of comments regarding the doc.

site/content/docs/5.1/components/orange-footer.md Outdated Show resolved Hide resolved
site/content/docs/5.1/components/orange-footer.md Outdated Show resolved Hide resolved
site/content/docs/5.1/components/orange-footer.md Outdated Show resolved Hide resolved
site/content/docs/5.1/components/orange-footer.md Outdated Show resolved Hide resolved
site/content/docs/5.1/components/orange-footer.md Outdated Show resolved Hide resolved
@julien-deramond

This comment was marked as outdated.

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.

Here is my design review after the last fixes we talked about. I compared directly the last example to the WEB-FTR-STD-003 reference. I used Firefox 95.0.1 (64-bit) for Ubuntu.

Sign up part

  • Every breakpoints: "Sign up to our mailing list" text is a bit off by 1px (top/bottom).
  • 768px breakpoint: Height is different than the one defined in the design sepcs but I would say the issue is in the design specs (check with the designers).
  • The behavior developed in this PR for the input + button seems good for me but is different than in the design specs where the width varies between breakpoints. Check with the designers what's the real behavior to develop. Note: in their 768px breakpoint the input is larger than in the 1024px and it looks like an issue.

Follow us part

  • Every breakpoints: "Follow us" text is a bit off by 1px (top/bottom).
  • 768px breakpoint: Height is different than the one defined in the design sepcs but I would say the issue is in the design specs (check with the designers).

Categories part

  • 480px: "Category" texts are a bit off by 1px (top/bottom).

Store locator part

  • Every breakpoints: "Locate a store" text is a bit off by 1px (top/bottom).
  • 768px breakpoint: Height is different than the one defined in the design sepcs but I would say the issue is in the design specs (check with the designers).
  • Icons + texts are a bit off (left/right) by few pixels

Copyright part

  • Icons + texts are a bit off (left/right) by few pixels
  1. Regarding the elements off by 1px pixel I let you check if you have the same behavior with Chrome and other browsers and if it worth a modification.
  2. Regarding the 768px multiple issues in the design specs I let you contact the designers to see if those are real issues
  3. Regarding the input size in the "Sign up" part I let you contact the designers and maybe show them what you've done and see if it is accepted or if you need to handle different widths.

Good job on the rendering. The result is already great as it is IMO.

I'll continue the review with the source code and the architecture while you're tackling those topics.

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.

Thanks for this work that is really needed to help all project implemented specific footers. I thinks there's maybe too many spacing and text sizing customization that will be loaded by all project but maybe not used. Need to find a good way to lightweight the overload and keep this footer Showroom

scss/_orange-footer.scss Outdated Show resolved Hide resolved
scss/_orange-footer.scss Outdated Show resolved Hide resolved
scss/_orange-footer.scss Outdated Show resolved Hide resolved
scss/_orange-footer.scss Outdated Show resolved Hide resolved
site/content/docs/5.1/components/orange-footer.md Outdated Show resolved Hide resolved
site/content/docs/5.1/components/orange-footer.md Outdated Show resolved Hide resolved
site/content/docs/5.1/components/orange-footer.md Outdated Show resolved Hide resolved
site/content/docs/5.1/components/orange-footer.md Outdated Show resolved Hide resolved
scss/_orange-footer.scss Outdated Show resolved Hide resolved
scss/_orange-footer.scss Outdated Show resolved Hide resolved
louismaximepiton and others added 23 commits April 22, 2022 11:34
Signed-off-by: louismaximepiton <louismaxime.piton@orange.com>
Signed-off-by: louismaximepiton <louismaxime.piton@orange.com>
Signed-off-by: louismaximepiton <louismaxime.piton@orange.com>
Signed-off-by: louismaximepiton <louismaxime.piton@orange.com>
Signed-off-by: louismaximepiton <louismaxime.piton@orange.com>
Signed-off-by: louismaximepiton <louismaxime.piton@orange.com>
Signed-off-by: louismaximepiton <louismaxime.piton@orange.com>
Signed-off-by: louismaximepiton <louismaxime.piton@orange.com>
Signed-off-by: louismaximepiton <louismaxime.piton@orange.com>
Signed-off-by: Isabelle Chanclou <isabelle.chanclou@orange.com>
Signed-off-by: Isabelle Chanclou <isabelle.chanclou@orange.com>
@julien-deramond julien-deramond self-requested a review April 22, 2022 09:57
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.

Good job! LGTM 🚀

@julien-deramond julien-deramond merged commit ac18dce into main Apr 22, 2022
@julien-deramond julien-deramond deleted the main-lmp-footer-examples branch April 22, 2022 10:55
@julien-deramond julien-deramond mentioned this pull request Apr 22, 2022
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.

Footer
7 participants