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-matomo-info)!: rework properties to avoid impossible states #1055

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-matomo-info component to implement our new state structure,
  • Implements TypeChecking within the cc-matomo-info component and its stories.

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.

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-matomo-info/state-migration/index.html.

This preview will be deleted once this PR is closed.

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. I have just some minor nitpicks.

src/templates/cc-link/cc-link.js Outdated Show resolved Hide resolved
src/templates/cc-link/cc-link.js Outdated Show resolved Hide resolved
@florian-sanders-cc florian-sanders-cc force-pushed the cc-matomo-info/state-migration branch 2 times, most recently from 1a8b1eb to 3bccb7f Compare May 13, 2024 13:29
@florian-sanders-cc florian-sanders-cc requested review from Galimede and roberttran-cc and removed request for Galimede May 13, 2024 14:21
@florian-sanders-cc florian-sanders-cc force-pushed the cc-matomo-info/state-migration branch 2 times, most recently from 91f9dd1 to b63a8da Compare May 20, 2024 09:18
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

BREAKING CHANGE: the properties have changed
- `state`: new property containing the whole state
- `matomoLink`: property has been deleted as it is now part of the state as `matomoUrl`
- `mysqlLink`: property has been deleted as it is now part of the state as `mysqlUrl`
- `phpLink`: property has been deleted as it is now part of the state as `phpUrl`
- `redisLink`: property has been deleted as it is now part of the state as `redisUrl`
- `error`: 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