Skip to content

Conversation

@Galimede
Copy link
Member

What does this PR do?

This PR introduces three new components for the new creation-tunnel:

  • cc-plan-item => The base plan card
  • cc-plan-picker => One plan picker
  • cc-plan-configurator => The form control element where you can refine your choice with two pickers

How to review?

  • Check the code
  • Check the stories

@Galimede Galimede force-pushed the tunnel-creation/plan-select branch 3 times, most recently from d201a79 to adaec03 Compare December 10, 2024 15:29
@github-actions
Copy link
Contributor

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

This preview will be deleted once this PR is closed.

@Galimede Galimede force-pushed the tunnel-creation/plan-select branch 2 times, most recently from caa8a97 to 6bcc75c Compare December 12, 2024 14:04
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 @Galimede the component looks good and the code is easy to read 👍

I left a few minor feedback and here are a few more:

  • When reviewing, I felt like the cc-plan-picker and cc-plan-configurator could be merged into a single component. cc-plan-picker could be a subrender that is nested only when there are relatedPlans. It's not that big of a deal, this is a nitpick and I'll leave it up to you ;),
  • Unless we're planning on using cc-plan-picker alone (but I don't see the usecase), I don't think we need to make cc-plan-picker a form element. Since its value is controlled by the parent anyway it doesn't bring any benefits.

Apart from that it's great!

@Galimede Galimede force-pushed the tunnel-creation/plan-select branch 2 times, most recently from e22b9fb to 3c40c29 Compare December 17, 2024 14:12
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.

I really like the design.

I feal like you could merge cc-plan-picker and cc-plan-configurator inside one single component (that could be named cc-plan-picker)

I also left some comments

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.

GG ! It looks very nice ! 👏
I have left some comments, mostly nitpicks.

@Galimede Galimede force-pushed the tunnel-creation/plan-select branch from f0a57d6 to 16c8890 Compare January 7, 2025 16:02
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.

Don't forget to add the cc-plan-item & cc-plan-picker components to the list of components to be typechecked by CI (in tsconfig.ci.json) 😉

Apart from that I only have a few nitpicks compared to my first review!

Gg @Galimede almost there 😎

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.

All good! Thanks & gg @Galimede

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 Mathieu. Almost there, I left some comments and nitpicks.

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.

Well done @Galimede, the API and types are really good. I mostly have issues/nitpicks about the CSS and UI.

cc-plan-item

  • when the name of the plan is long, it does not wrap
    • we will try to avoid such situations but the component should still work properly if it happens
    • we need to story to demonstrate such a case: long plan name and custom CSS for story with a small width on the component (like when it's used in the picker).
  • I wrote a lot of comments about your flexbox usage, but here's one that is a bit more global: the display: flex on the :host is not really used (same for the two flex: xx on .title and .details. They could/should be useful in the picker so all cc-plan-item have the same height and the .title takes the rest of the height`
    • This requires a change in the picker
    • The flex: xx on .title and .details would still need to be fixed
  • the "selected" icon can sometimes go above the plan name or badge, is this a problem?

cc-plan-picker

  • like we need a story in cc-plan-item for long plan names, we need a story in this one to show how a long name will wrap, grow the height of the plan item and the other on the same row.

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.

Looking good! A few minor feedback on my side.

Also, I observed a weird behavior with the plan picker & keyboard focus but it may be legit:

  • testing a story with related plans
  • choosing another "parent" plan with the right arrow (you should just not select the last "parent" plan)
  • focus is now on the new value
  • pressing tab
  • the focus is on the "related" plans, the first one
  • pressing shift+tab to focus back to the "parent" plans
  • I expect to be focused on the selected plan but I'm focused on the last "parent" plan

@Galimede Galimede force-pushed the tunnel-creation/plan-select branch 3 times, most recently from 48ad192 to 321fa39 Compare January 24, 2025 10:17
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.

I had a quick look at the "hubert feedbacks", LGTM 👍 Well done!

@Galimede Galimede force-pushed the tunnel-creation/plan-select branch from 321fa39 to 90e054f Compare January 28, 2025 15:14
@Galimede Galimede force-pushed the tunnel-creation/plan-select branch from 90e054f to e262014 Compare January 28, 2025 15:46
@Galimede Galimede merged commit e5f1b1f into master Jan 28, 2025
4 checks passed
@Galimede Galimede deleted the tunnel-creation/plan-select branch January 28, 2025 16:11
@github-actions
Copy link
Contributor

🔎 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.

6 participants