-
Notifications
You must be signed in to change notification settings - Fork 76
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
News Site Nuxt - Article Ids #448
base: main
Are you sure you want to change the base?
News Site Nuxt - Article Ids #448
Conversation
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.
The general approach looks good to me, but we need to avoid touching the original singleton to avoid surprises in the future.
Using map
with some object spread operators should also make it more explicit that we're not touching it.
Thanks!
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.
thanks!
note you'll need to rebase and regenerate the dist
const currentItem = { ...item }; | ||
currentItem.id = uuidv4(); | ||
return currentItem; |
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.
I wanted to (optionally) suggest this:
const currentItem = { ...item }; | |
currentItem.id = uuidv4(); | |
return currentItem; | |
return { | |
...item, | |
id: uuidv4(); | |
}; |
but I'm not sure this looks better.
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.
I know what you mean. I think I'd rather keep things consistent (visually) and treat currentArticle and currentItem the same way.
2d1c36f
to
3fb9c89
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.
Seems sane to me.
This is an alternate solution for: #427
The reason to keep creating unique ids, is that it'll allow us to potentially opt into a complexity param (#430), which could append content by cloning the sections in the data file. This is only possible, if we don't hard-code list ids in the data file (which was the previous solution).
Here, I am just creating the unique ids once, when the data gets assigned to the app, instead of the currently used version that had issues (#422).
Difference to the Next.js version:
You'll notice that the article-content already uses article.id in some instances in the main branch - this is not throwing any errors, but does assign
undefined
, since the article.id doesn't exist unless we create it somewhere (which is now done in the pr branch).note: feel free to ignore the dist folder - that's just the generated build files
Crossbench - 10 iterations:
this pr
main