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.

Sign up for GitHub

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

Jump to bottom

feat(creation-tunnel): introduce cc-zone-card and cc-zone-picker components #1154

Merged
merged 3 commits into from
Oct 9, 2024
Merged

feat(creation-tunnel): introduce cc-zone-card and cc-zone-picker components #1154

merged 3 commits into from
Oct 9, 2024

Conversation

Galimede
Copy link
Member

@Galimede Galimede commented Sep 18, 2024 •
edited
Loading

What does this PR do?

  • This PR introduces two new components cc-zone-card and cc-zone-picker:
    • It uses ElementInternals for cc-zone-picker and acts as a radio form input
    • It will be used as the zone choice in both the creation tunnel and later on on the information page of a product
    • Note: there's no skeleton nor error state as there's a loader in the tunnel, thus it doesn't make sense to have those states for now

How to review?

  • Check the code
  • Check the preview:
    • stories
    • both French & English
    • different viewport sizes
    • keyboard input/focus

@Galimede Galimede force-pushed the tunnel-creation/zone-select-two branch 4 times, most recently from 6a0546c to 07f40e4 Compare September 18, 2024 15:29
@github-actions GitHub Actions
Copy link
Contributor

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

This preview will be deleted once this PR is closed.

pdesoyres-cc
pdesoyres-cc requested changes Sep 19, 2024
View reviewed changes
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.

Well done @Galimede, those two components are very nice.

I'm triggered by the introduction of a new font, but maybe this is something that has already been acted during your investigations with @bob

I also left some comments.

src/components/cc-zone-picker/cc-zone-picker.js Outdated Show resolved Hide resolved
src/components/cc-zone-picker/cc-zone-picker.js Outdated Show resolved Hide resolved
src/components/cc-zone-card/cc-zone-card.js Outdated Show resolved Hide resolved
src/components/cc-zone-card/cc-zone-card.js Outdated Show resolved Hide resolved
src/components/cc-zone-card/cc-zone-card.js Outdated Show resolved Hide resolved
src/components/cc-zone-card/cc-zone-card.js Outdated Show resolved Hide resolved
florian-sanders-cc
florian-sanders-cc requested changes Sep 26, 2024
View reviewed changes
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.

🎉 Great job @Galimede this component looks great and it's very easy to use / understand.

I think it's API is dead simple (great thing) and the fact it relies on ElementInternals is 😍

I've left a few comments but we're almost there, don't hesitate to ping me if something is unclear or if you want to discuss some points 😉

src/components/cc-zone-card/cc-zone-card.js Outdated Show resolved Hide resolved
src/components/cc-zone-card/cc-zone-card.js Outdated Show resolved Hide resolved
src/components/cc-zone-card/cc-zone-card.js Outdated Show resolved Hide resolved
src/components/cc-zone-card/cc-zone-card.js Outdated Show resolved Hide resolved
src/components/cc-zone-card/cc-zone-card.stories.js Outdated Show resolved Hide resolved
src/components/cc-zone-picker/cc-zone-picker.js Outdated Show resolved Hide resolved
src/components/cc-zone-picker/cc-zone-picker.js Outdated Show resolved Hide resolved
src/components/cc-zone-picker/cc-zone-picker.js Outdated Show resolved Hide resolved
src/components/cc-zone-picker/cc-zone-picker.stories.js Show resolved Hide resolved
src/components/cc-zone-picker/cc-zone-picker.js Show resolved Hide resolved
HeleneAmouzou
HeleneAmouzou approved these changes Sep 26, 2024
View reviewed changes
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 components look great ! 👏
I just left a comment below !

src/components/cc-zone-picker/cc-zone-picker.js Outdated Show resolved Hide resolved
hsablonniere
hsablonniere requested changes Oct 2, 2024
View reviewed changes
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 @Galimede, well done on this one 👍

The code is really easy and nice to read and review. Good job!! ❤️

  • I like the different widths in stories for the card
  • Should we add title for tooltips on country flags and infra logo?

src/components/cc-zone-card/cc-zone-card.js Outdated Show resolved Hide resolved
src/components/cc-zone-card/cc-zone-card.js Outdated Show resolved Hide resolved
src/components/cc-zone-card/cc-zone-card.js Outdated Show resolved Hide resolved
src/components/cc-zone-card/cc-zone-card.js Outdated Show resolved Hide resolved
src/components/cc-zone-card/cc-zone-card.js Outdated Show resolved Hide resolved
src/components/cc-zone-picker/cc-zone-picker.js Outdated Show resolved Hide resolved
src/styles/default-theme.css Outdated Show resolved Hide resolved
src/components/cc-zone-card/cc-zone-card.js Outdated Show resolved Hide resolved
src/components/cc-zone-card/cc-zone-card.js Outdated Show resolved Hide resolved
src/components/cc-zone-picker/cc-zone-picker.stories.js Outdated Show resolved Hide resolved
@Galimede Galimede force-pushed the tunnel-creation/zone-select-two branch 4 times, most recently from 55bf255 to ea251f7 Compare October 8, 2024 09:04
pdesoyres-cc
pdesoyres-cc approved these changes Oct 8, 2024
View reviewed changes
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.

LGTM

roberttran-cc
roberttran-cc approved these changes Oct 8, 2024
View reviewed changes
Copy link
Member

@roberttran-cc roberttran-cc left a comment

Choose a reason for hiding this comment

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

Great job!
I have a minor issue & a suggestion in this review.
On top of that, I'll push a commit to propose small tuning of the UI (on the spacing): it's up to you if you want to keep it!

src/components/cc-zone-picker/cc-zone-picker.js Outdated Show resolved Hide resolved
src/components/cc-zone-picker/cc-zone-picker.js Outdated Show resolved Hide resolved
@roberttran-cc roberttran-cc force-pushed the tunnel-creation/zone-select-two branch from 7c9cb2d to eec800e Compare October 8, 2024 16:28
@Galimede Galimede force-pushed the tunnel-creation/zone-select-two branch from eec800e to dd301a3 Compare October 9, 2024 08:19
florian-sanders-cc
florian-sanders-cc approved these changes Oct 9, 2024
View reviewed changes
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.

Great job again @Galimede, all good!

@Galimede Galimede force-pushed the tunnel-creation/zone-select-two branch from dd301a3 to 07517d6 Compare October 9, 2024 09:29
@Galimede Galimede merged commit 5262f07 into master Oct 9, 2024
4 checks passed
@Galimede Galimede deleted the tunnel-creation/zone-select-two branch October 9, 2024 09:35
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Oct 9, 2024

🔎 The preview has been automatically deleted.

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

@HeleneAmouzou HeleneAmouzou HeleneAmouzou approved these changes

@pdesoyres-cc pdesoyres-cc pdesoyres-cc approved these changes

@roberttran-cc roberttran-cc roberttran-cc approved these changes

@florian-sanders-cc florian-sanders-cc florian-sanders-cc approved these changes

@hsablonniere hsablonniere hsablonniere approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Galimede @hsablonniere @florian-sanders-cc @roberttran-cc @pdesoyres-cc @HeleneAmouzou