Navigation: add unit tests and refactor AMP constants #4968
Conversation
tochwill
changed the title
tochwill
requested review from
OlgaLyubin,
AlistairGempf,
DenisHdz,
Bopchy,
a user and
j-pendlebury
| const dropdownListItems = ( | ||
| // Hidden attribute is added if we're on amp because the amp toggleVisibility |
There was a problem hiding this comment.
This comment was added by Alistair after the swarm review to explain the usage of the hidden attribute. I would keep it and put it before the {cloneElement(dropdownListItems, { id: DROPDOWN_ID, hidden: true })} .
There was a problem hiding this comment.
Sure, I'll add a comment back in :)
| <Navigation | ||
| scrollableListItems={scrollableListItems} | ||
| dropdownListItems={dropdownListItems} | ||
| skipLinkText="skip link" |
There was a problem hiding this comment.
We don't need to pass the Skip link to the Navigation anymore, since it will be handled by the Brand.
There was a problem hiding this comment.
You can remove it from the NavigationContainer, but still, we don't need to pass it in this PR.
There was a problem hiding this comment.
Good spot - I missed that this was still being passed.
| dir="ltr" | ||
| /> | ||
| ); | ||
|
|
There was a problem hiding this comment.
Could we put scrollableTestId, dropdownTestId, scrollableListItems, dropdownListItems and navigation in a file, and imported them here and in src/app/containers/Navigation/index.amp.test.jsx - as they are mostly similar?
Co-Authored-By: sadickisaac <[email protected]>
|
@sadickisaac we left the brand container test failing in the feature branch, to ensure that we couldn't merge this into latest until we added the skip to content link back into the brand container |
|
@tochwill Ok. That makes sense. |
Resolves #NUMBER
Overall change:
Writes unit tests for the navigation, and applies code review feedback for AMP constants and converting repeated component composition into a reusable function.
Code changes:
Testing:
CYPRESS_APP_ENV=local CYPRESS_SMOKE=false npm run test:e2e:interactive)