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

Example: New e-shop example #1839

Closed
wants to merge 35 commits into from
Closed

Conversation

louismaximepiton
Copy link
Member

@louismaximepiton louismaximepiton commented Feb 15, 2023

Note: Please transform - [ ] into - (NA) in the description when things are not applicable

Related issues

Closes #1560.

Description

Things to know before reviewing it

  • The tags should wrap on 2 lines at max and have a horizontal scroll padding.
  • The phone images should respect a ratio of 3/4 and have a width of 4 columns of its parent.
  • Spacings between texts inside the main phone cards are 20px above, 5px between From and orange price and 0px between this price and the optionnal one under.
  • There are some issues with the font styles. They will mainly be tackled with font tokens once we implement them.
  • Many spacings will need to be reworked once the spacing tokens will be implemented.
  • The counter component will maybe be discussed inside a spec meeting to determine the behavior, the a11y and the different designs it could take.
  • Fine to write on .bg-danger
  • .border-supporting-* shouldn't exist since its example related. They have been transformed into .border-success and .border-info.
  • The design for the main phone cards as been a bit tweaked to make sure to have the same (or almost) markup in each card (the checkbox and button should appear 20px under the bottomest element above).
  • The stretched links should be used only if there's only one thing to handle. There should be non cliquable elements around those.
  • I've added some skip links, feel free to add some if you can see some useful others.
  • I've added a back-to-top component.
  • Some colors have to be tweaked a bit since the design changed some color tokens.

Things to tackle outside of this PR

Motivation & Context

Types of change

  • New feature (non-breaking change which adds functionality)

Live previews

Checklist

Contribution

Accessibility

  • My change follows accessibility good practices; I have at least run axe

Design

  • My change respects the design guidelines defined in Orange Design System
  • My change is compatible with responsive display

Development

  • My change follows the developer guide
  • (NA) I have added JavaScript unit tests to cover my changes
  • (NA) I have added SCSS unit tests to cover my changes

Documentation

  • My change introduces changes to the documentation and/or I have updated the documentation accordingly

Checklist (for Core Team only)

  • My change introduces changes to the migration guide
  • (NA) My new component is added in Storybook
  • (NA) My new component is compatible with RTL
  • Manually run BrowserStack tests
  • Manually test browser compatibility with BrowserStack (Chrome >= 60, Firefox >= 60 (+ ESR), Edge, Safari >= 12, iOS Safari, Chrome & Firefox on Android)
  • Code review
  • Design review
  • A11y review

After the merge

@netlify
Copy link

netlify bot commented Feb 15, 2023

Deploy Preview for boosted ready!

Name Link
🔨 Latest commit 332747a
🔍 Latest deploy log https://app.netlify.com/sites/boosted/deploys/6554854d7c94980008def3a9
😎 Deploy Preview https://deploy-preview-1839--boosted.netlify.app/docs/5.3/examples/e-shop
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@louismaximepiton louismaximepiton marked this pull request as ready for review March 21, 2023 10:11
@louismaximepiton
Copy link
Member Author

For instance, I think I won't fix 2, 3 since those will introduce more unusable javascript for projects, 5 because we don't have it now. I'll update the corresponding issue to not forget it. 8, 10 will probably be tackled with dark mode.

<li class="card border-0 pt-4 pt-md-4">
<h3 class="visually-hidden">xxxx xxx</h3>
<div class="d-flex mt-md-3 mb-2 align-items-end justify-content-between border-bottom border-danger fw-bold">
<div class="d-flex mt-md-3">
Copy link
Member Author

Choose a reason for hiding this comment

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

Not a big fan of those .mt-md-3 and mt-xl-3 since they are quite too manual imo. At least we get the same rendering as the design.

@CyriaqueBillard
Copy link
Member

Thank you for the latest updates. @louismaximepiton
Validated.

@julien-deramond
Copy link
Member

@MewenLeHo Could please you finish the review on your side?

MewenLeHo

This comment was marked as outdated.

@louismaximepiton
Copy link
Member Author

Corrected via 301d495.

I'm a bit confused here, it seemed to happen because the elements positioned in absolute were outside of the screen. However, setting position: relative anywhere else doesn't correct the issue. So maybe it's only a bug with the interaction between overflow and positioning ?

Copy link
Contributor

@MewenLeHo MewenLeHo left a comment

Choose a reason for hiding this comment

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

Not sure if it was a bug or if we missed an interaction in the code 🤔
But the fix is simple and clean so good for me.

@Aniort
Copy link
Contributor

Aniort commented Nov 8, 2023

Hello,
just 2 little issues:

  • The DAS info must be in a <p>.
  • The "Find yours" button must be self-explaining, one solution could be:
<div class="col-12 col-lg-5">
            <div class="h-100 p-2 p-lg-4 bg-dark">
              <div class="pt-1 pt-lg-0 pe-3 pe-lg-4 pb-1 ps-2">
                <h3 class="h1 mb-2 text-primary" **id=bidule"**>The best for my budget</h3>
                <p class="h2 pt-1 pt-lg-2 mb-2">iPhone at a reduced price refurbished, with 1 year guarantly</p>
                <p class="h5 pt-1 pt-lg-2 mb-3 text-primary">From 850 000 FCFA</p>
                <a href="#" class="btn btn-secondary btn-inverse mt-1 mt-lg-2 mb-2" **id="truc" aria-labelledby="truc bidule"**>Find yours</a>
              </div>
            </div>
          </div>

Copy link
Contributor

@Aniort Aniort left a comment

Choose a reason for hiding this comment

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

hello,
just invert the 2 id:

aria-labelledby="best-budget-FIND best-budget-TITLE"
and it'll be fine !

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

Successfully merging this pull request may close these issues.

E-Shop - Devices list
5 participants