-
Notifications
You must be signed in to change notification settings - Fork 18
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-tile-requests)!: rework properties to avoid impossible states #1057
base: master
Are you sure you want to change the base?
Conversation
🔎 A preview has been automatically published : https://clever-components-preview.cellar-c2.services.clever-cloud.com/cc-tile-requests/state-migration/index.html. This preview will be deleted once this PR is closed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well done, but see mmy comment bellow.
42e50e5
to
dab6d06
Compare
Here are some thoughts on the empty state
Conclusion/TL;DR: In my opinion, there's no correct answer. However, I do like the Maybe, if we had to make a compromise we could do it the other way around first and when we'll have a smart someday, go for this solution. 🤷♂️ I'm sorry @florian-sanders-cc and @HeleneAmouzou that's not a clear yes or no. 😅 |
Sync about
|
23203eb
to
b2a69a8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
b2a69a8
to
bc0903b
Compare
…ates BREAKING CHANGE: the properties have changed - `state`: new property containing the whole state - `error`: property has been deleted as it is now part of the state - `data`: property has been deleted as it is now part of the state
77b9d6b
to
6714ba2
Compare
What does this PR do?
cc-tile-requests
component to implement our new state structure,cc-tile-requests
component and its stories.How to review?