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

refactor(cc-addon-admin)!: rework properties to avoid impossible states #1044

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

florian-sanders-cc
Copy link
Contributor

@florian-sanders-cc florian-sanders-cc commented May 7, 2024

What does this PR do?

  • Refactors the cc-addon-admin component to implement our new state structure,
  • Implements TypeChecking within the cc-addon-admin component and its stories,
  • Adds 2 new stories because we handle name / tag updates a little bit differently: we now set the related submit button in waiting mode ("update name" button for name update for instance) and disable other buttons while waiting,
  • Updates the simulation story to reflect the new stories (simulates a name update, then a tag update, then an add-on deletion).

How to review?

  • Check the commit (the diff may not be easy to read so you can review the final result locally if you prefer),
  • If you check it locally, you should not get any Typescript error within the component file or the story file,
  • Compare the prod stories to the preview stories.

@florian-sanders-cc florian-sanders-cc marked this pull request as ready for review May 7, 2024 07:43
Copy link
Contributor

github-actions bot commented May 7, 2024

🔎 A preview has been automatically published : https://clever-components-preview.cellar-c2.services.clever-cloud.com/cc-addon-admin/state-migration/index.html.

This preview will be deleted once this PR is closed.

@florian-sanders-cc florian-sanders-cc added this to the State migration milestone May 7, 2024
@florian-sanders-cc florian-sanders-cc force-pushed the cc-addon-admin/state-migration branch 3 times, most recently from fb95c39 to 2bde9af Compare May 13, 2024 14:15
@florian-sanders-cc florian-sanders-cc requested review from Galimede, hsablonniere and roberttran-cc and removed request for Galimede May 13, 2024 14:31
@florian-sanders-cc florian-sanders-cc force-pushed the cc-addon-admin/state-migration branch 2 times, most recently from b6a99dd to ad70a26 Compare May 20, 2024 09:21
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 @florian-sanders-cc and @HeleneAmouzou, thank you for this 🎉

I added a few comments. I wanted to suggest to inline the subrender method but looking at its size and the number of variables it computes, I guess the current code is better.

Well done!

src/components/cc-addon-admin/cc-addon-admin.js Outdated Show resolved Hide resolved
src/components/cc-addon-admin/cc-addon-admin.types.d.ts Outdated Show resolved Hide resolved
BREAKING CHANGE: the properties have changed
- `state`: new property containing the whole state
- `addon`: property has been deleted as it is now part of the state
- `error`: property has been deleted as it is now part of the state
- `saving`: property has been deleted as it is now part of the state
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.

None yet

4 participants