refactor(cc-article-components)!: rework properties to avoid impossible states #1054
Conversation
|
🔎 A preview has been automatically published : https://clever-components-preview.cellar-c2.services.clever-cloud.com/cc-article-list/state-migration/index.html. This preview will be deleted once this PR is closed. |
florian-sanders-cc
changed the title
| export interface ArticleCardStateLoaded { | ||
| type: 'loaded'; | ||
| banner: string; | ||
| date: string; | ||
| description: string; | ||
| link: string; | ||
| title: string; | ||
| } |
There was a problem hiding this comment.
| export interface ArticleCardStateLoaded { | |
| type: 'loaded'; | |
| banner: string; | |
| date: string; | |
| description: string; | |
| link: string; | |
| title: string; | |
| } | |
| export interface ArticleCardStateLoaded extends SkeletonArticle { | |
| type: 'loaded'; | |
| } |
If we do that, we should rename SkeletonArticle interface into Article.
| type: 'error'; | ||
| export interface ArticleListStateLoaded { | ||
| type: 'loaded'; | ||
| articles: ArticleCardStateLoaded[]; |
There was a problem hiding this comment.
nitpick: I'm ok with that but when I look at the smart component, I can see that all articles are fetch in one call. So there is no need to provide a state but only the data.
florian-sanders-cc
changed the title
florian-sanders-cc
force-pushed
the
cc-article-list/state-migration
branch
from
2e30bba
to
99185ac
Compare
| <div class="title"> | ||
| ${ccLink(this.link, title, skeleton)} | ||
| ${ccLink(this.state.link, data.title)} |
There was a problem hiding this comment.
suggestion: while this.state.link is valid, you could use data.link to make it less confusing
| export interface ArticleCardStateLoaded { | ||
| type: 'loaded'; | ||
| banner: string; | ||
| date: string; | ||
| description: string; | ||
| link: string; | ||
| title: string; | ||
| } |
| const rawArticleListData = parseRssFeed(rssFeed, limit); | ||
|
|
||
| return rawArticleListData.map((rawArticle) => ({ | ||
| type: 'loaded', |
There was a problem hiding this comment.
thought: Okay, I think this line makes me question having a state for cc-article-card. My "problem" is that cc-article-card is not loading data directly, it will be the role of this smart for cc-article-list. It feels like managing two states here. Plus, as we'll only use the card component within the list it might be overkill 🤔. Feel free to ping me if it's unclear for you. 😉
florian-sanders-cc
force-pushed
the
cc-article-list/state-migration
branch
from
92a6cda
to
3dab4cf
Compare
BREAKING CHANGE: the properties have changed - `state`: new property containing the whole state - `title`: property has been deleted as it is now part of the state - `description`: property has been deleted as it is now part of the state - `banner`: property has been deleted as it is now part of the state - `date`: property has been deleted as it is now part of the state - `link`: property has been deleted as it is now part of the state
florian-sanders-cc
force-pushed
the
cc-article-list/state-migration
branch
from
7cdfdc8
to
ecd09d6
Compare
What does this PR do?
cc-article-list&cc-article-cardcomponents to implement our new state structure,cc-article-list&cc-article-cardcomponents and their stories.How to review?