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

feat(creation-tunnel): init order summary component #1305

Merged
merged 1 commit into from
Feb 4, 2025

Conversation

roberttran-cc
Copy link
Member

Fixes #1304

Quite a simple review

  • check the code
  • check the preview

Copy link
Contributor

🔎 A preview has been automatically published : https://clever-components-preview.cellar-c2.services.clever-cloud.com/tunnel-creation/order-summary/index.html.

This preview will be deleted once this PR is closed.

@roberttran-cc roberttran-cc force-pushed the tunnel-creation/order-summary branch 3 times, most recently from c5e6d5d to 00ace19 Compare January 24, 2025 08:43
@roberttran-cc roberttran-cc marked this pull request as ready for review January 24, 2025 08:50
@roberttran-cc roberttran-cc force-pushed the tunnel-creation/order-summary branch from 00ace19 to 32ebcb7 Compare January 24, 2025 08:53
Copy link
Contributor

@pdesoyres-cc pdesoyres-cc left a comment

Choose a reason for hiding this comment

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

Nice component Bob. I really like the design. Praise for the css and html organization, it makes the code really clear and easy to read.

I left some minor comments but nothing blocking.

@roberttran-cc roberttran-cc force-pushed the tunnel-creation/order-summary branch from 32ebcb7 to 54ba53d Compare January 27, 2025 10:22
Copy link
Member

@hsablonniere hsablonniere left a comment

Choose a reason for hiding this comment

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

Hey @roberttran-cc, well done!!! Just a few notes and nitpicks 😉

Copy link
Contributor

@florian-sanders-cc florian-sanders-cc left a comment

Choose a reason for hiding this comment

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

Well done @roberttran-cc the component looks really good.

I didn't expect it to feel so easy to read / lightweight so great job again!
Also, the code is very clean and easy to read ❤️

Some of the points I raised will need to be handled before the final release (out of beta I mean): the logo alternative text, the data list semantics.
Other points are really nitpicks or suggestions; they can easily be ignored / dismissed.

I have a few accessibility observations that are non blocking:

  • the alignment of labels on the left and values on the right is not screen magnifier friendly. Some people using these tools will struggle. It's non blocking because placing labels next to values is only a requirement in forms + I don't really have a better idea that would look ok in this case. Also the distance is not that big (very very subjective) 🤷. It's just something we should avoid as much as possible but here I think it's ok?
  • Placing text after (in the dom order) the create button is not fobidden but it should be discouraged because it's highly possible that people using screen readers won't even notice that it's there. Usually the "submit button" is the last part of a form and people don't expect to have to browse past it. We could move it within the DOM and keep the visual order, especially since it contains no focusable elements but let's wait and see how it evolves and what we want to show in that section exactly before deciding? Let's just keep that in mind.

Feel free to ping me if some stuff is unclear 😉

Copy link
Contributor

@HeleneAmouzou HeleneAmouzou left a comment

Choose a reason for hiding this comment

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

Well done, the summary looks nice and it will sure be very useful ! 👏 I did not leave any comments, but I have overall questions:

  • Have you considered using a cc-block to render the summary ? If so, why choosing not to use it ?
  • Why does the component only have one state ? Is it planned to have a loading and an error states in the future ?

Thanks in advance !

@roberttran-cc roberttran-cc force-pushed the tunnel-creation/order-summary branch 2 times, most recently from b9fa770 to 5044740 Compare January 30, 2025 15:52
@roberttran-cc
Copy link
Member Author

Thank you so much for your reviews! 🫶
They all led to some valuable modifications (more than I expected initially).

@florian-sanders-cc & @HeleneAmouzou: about the state property
Actually, I didn't give a thought beforehand, it was the force of habit. 😅
But the attempts to integrate the component in the console have shown that the state may turn out to be useful. But for the time being, I renamed the state variable but did not flatten the properties. We'll come back on this with more insights: hopefully, the component is in beta!

@HeleneAmouzou: about the cc-block usage
I actually did not think about it. 😬
The main reason probably is because the component is strongly opinionated design-wise and not exactly what pictured the prototype. Another point is that the order summary would not use all the options offered by the cc-block (at first glance): therefore, it would bring some unnecessary code.
But feel free to get back to me if you have something in mind!

@florian-sanders-cc: on label alignment and screen magnifier
It is a tricky one, and I don't have a proper solution that would fix this issue while retaining the design.
Meanwhile, I pushed some slight UI updates that may alleviate the issue (commit fix: feedback Florian (more readable informations for a11y): I'd love your feedback on this.
Globally, we need to discuss this topic synchronously (with Marion), as it will come back regularly.

@florian-sanders-cc: on the text after the submit button
Another tricky one! 😅
The issue here is that the details is out of the container where belong the submit button. Again, I don't have a solution I like but I'll keep it in mind and we will discuss this synchronously as well.

@florian-sanders-cc
Copy link
Contributor

@florian-sanders-cc & @HeleneAmouzou: about the state property Actually, I didn't give a thought beforehand, it was the force of habit. 😅 But the attempts to integrate the component in the console have shown that the state may turn out to be useful. But for the time being, I renamed the state variable but did not flatten the properties. We'll come back on this with more insights: hopefully, the component is in beta!

Yes this was really a nitpick, no problem 👍.
And if we go back to using a state then it's even better.

@florian-sanders-cc: on label alignment and screen magnifier It is a tricky one, and I don't have a proper solution that would fix this issue while retaining the design. Meanwhile, I pushed some slight UI updates that may alleviate the issue (commit fix: feedback Florian (more readable informations for a11y): I'd love your feedback on this. Globally, we need to discuss this topic synchronously (with Marion), as it will come back regularly.

Yes this is really tricky and I don't have a good solution here. I think your modifications make it better and I think we can stick with that. To be honest my comment was more about raising the concern in general than fixing it in this specific case because I can't think of a better design 👍

@florian-sanders-cc: on the text after the submit button Another tricky one! 😅 The issue here is that the details is out of the container where belong the submit button. Again, I don't have a solution I like but I'll keep it in mind and we will discuss this synchronously as well.

I think we need to iterate a bit and see the component in its context before deciding whether we want to change this or leave it as is. The goal of my comment was to raise the issue so we don't forget it in the long run so don't worry, it's perfectly ok to keep it as it is (and it might even remain like that when we go to prod, we'll discuss synchronously as you suggested 👍

Copy link
Contributor

@florian-sanders-cc florian-sanders-cc left a comment

Choose a reason for hiding this comment

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

I've checked the feedback commits where I'm mentionned + the stories (quickly), it's all good for me!

Great job @roberttran-cc !!

@HeleneAmouzou
Copy link
Contributor

@roberttran-cc Thanks for the detailed answers ! I have checked stories and LGTM !

@roberttran-cc roberttran-cc force-pushed the tunnel-creation/order-summary branch from 5044740 to 1defa80 Compare February 4, 2025 13:09
@roberttran-cc roberttran-cc merged commit 37606f2 into master Feb 4, 2025
4 checks passed
@roberttran-cc roberttran-cc deleted the tunnel-creation/order-summary branch February 4, 2025 13:18
Copy link
Contributor

github-actions bot commented Feb 4, 2025

🔎 The preview has been automatically deleted.

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

Successfully merging this pull request may close these issues.

cc-order-summary: init component
5 participants