rename industry to principles #376
Conversation
Tweaks the menu text for the first two columns as well, still working on the third one. Closes #339
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/ocaml/v3-ocaml-org/7gtmMMDE5naLPTwpTBzn36awBYDo |
src/HeaderNavigation.res
Outdated
| @@ -324,12 +324,12 @@ let make = (~content) => { | |||
| section.entries, | |||
| ), | |||
| [ | |||
| <h3 className="ml-6 mt-2 px-3 font-semibold text-gray-400 uppercase"> | |||
| <h3 key="0" className="ml-6 mt-2 px-3 font-semibold text-gray-400 uppercase"> | |||
There was a problem hiding this comment.
This is needed to suppress a warning in the HeaderNavigation, cc @jonludlam
Warning: Each child in a list should have a unique "key" prop.
Check the top-level render call using <nav>. See https://reactjs.org/link/warning-keys for more information.
at h3
at HeaderNavigation (webpack-internal:///./src/HeaderNavigation.js:17:23)
at div
at div
at MainLayout (webpack-internal:///./src/MainLayout.js:19:24)
at make (webpack-internal:///./src/App.js:15:25)
at AppContainer (/Users/avsm/src/git/ocaml/v3.ocaml.org/node_modules/next/dist/next-server/server/render.js:28:861)
There was a problem hiding this comment.
We should probably restructure this so that only one key is needed, but you can just add a TODO in the source code, and one of us will do that when working on this source code in the future.
There was a problem hiding this comment.
It's just a single-item constant list though; do we need to do anything?
There was a problem hiding this comment.
If I'm not mistaken, the key is needed whenever you have a list of React elements, regardless of whether the list contains one or more elements.
There was a problem hiding this comment.
Yes, that's why I added a key="0" in this PR :-)
There was a problem hiding this comment.
Ok, I just had a second look at this code, and it became apparent to me that the key value you've used here would collide with the other siblings in the array. The key should actually be its index in the array, which in this case, is the final element, not the 0th element.
That being said, I think the whole block of code could be refactored to make it a more visually apparent that this is the case.
There was a problem hiding this comment.
Looking at it even more closely, it seems the even original implementation is incorrect. From what I can tell, the index is restarting from 0 for every nested array, and then the nesting is being flattened. That would indicate to me that there are 4 instances of key="0" here.
There was a problem hiding this comment.
Is there a way to add 'key' to the items after they're created? Seems easier to create the list, then once it's completely made do a final mapi to add the keys on.
There was a problem hiding this comment.
My best thought at the moment which doesn't involve threading an accumulator through a reduce function, or using a ref, would be to map each array element to a function of type int => React.element, and then do the Js.Array.mapi call at the end.
|
It's missing a rename of |
|
Should be good for review now. |
Followup issue is #382 to fix this
Tweaks the menu text for the first two columns as well, still
working on the third one.
Closes #339