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

News Site Next #426

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

flashdesignory
Copy link
Contributor

This fixes next.js issue: #422
A separate pr for Nuxt will do the same.

The fix here is to add unique ids to any article content that is a list.
This is done in the actual data files, so no need for the app to generate them at any point.

@kara

Copy link
Contributor

@camillobruni camillobruni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally LGTM: might be worth adding a little test to check for consistency across languages and uniqueness of IDs within a language.

@flashdesignory
Copy link
Contributor Author

yes, absolutely!
Tests are needed and will come in a future PR at some point.

Copy link
Contributor

@julienw julienw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me, thanks for the fix.
One more thing: in section.jsx you can use article.id instead of concatenating the section id with index.

@rniwa
Copy link
Member

rniwa commented Sep 4, 2024

Do we have before/after numbers?

@flashdesignory
Copy link
Contributor Author

Do we have before/after numbers?

here's a snapshot

browser before after
chrome 72.27 70.27
firefox 77.47 72.43
safari 54.03 57.47

@rniwa
Copy link
Member

rniwa commented Sep 4, 2024

Is that runtime or pts?

@flashdesignory
Copy link
Contributor Author

Is that runtime or pts?

we're displaying runtime in the detail screen, right?

@flashdesignory
Copy link
Contributor Author

@rniwa - any concerns to merge this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants