feat: Page template updates. #8
Conversation
| '@typescript-eslint/ban-types': 'off', | ||
| '@typescript-eslint/consistent-type-assertions': 'error', | ||
| '@typescript-eslint/consistent-type-definitions': 'error', | ||
| '@typescript-eslint/consistent-type-definitions': 'off', |
There was a problem hiding this comment.
Just curious on the "why" behind this eslint config change
| <KebabDropdownItems /> | ||
| </DropdownList> | ||
| </Dropdown> | ||
| <GithubIcon /> |
There was a problem hiding this comment.
Nit: I think the icon should be passed to an icon prop rather than as a child
| const latestRelease = versions.Releases.find( | ||
| (release) => release.latest, | ||
| ) as Release | ||
| const previousReleases = Object.values(versions.Releases).filter( | ||
| (release) => !release.hidden && !release.latest, | ||
| ) as Release [] | ||
|
|
||
| const previousVersions = Object.values(versions.ArchivedReleases) as Release[]; |
There was a problem hiding this comment.
Nit: is there a way we could avoid all these type casts?
| itemId={`${version.name}-latest-release`} | ||
| key={`${version.name}-latest`} | ||
| // eslint-disable-next-line no-nested-ternary | ||
| to={isLatest ? '/' : version.href ? version.href : `/${version.name}`} |
There was a problem hiding this comment.
Nit: I don't love this nested ternary either
| <DropdownGroup key="Previous releases" label="Previous releases"> | ||
| <DropdownList> | ||
| {previousReleases | ||
| .slice(0, 3) |
There was a problem hiding this comment.
Nit: is this slice just a magic number sort of thing where we're grabbing the last three versions just because 3, 4 and 5 are the only previous versions we care about? If so I don't love that.
|
|
||
| fireEvent.click(screen.getByText('Release 1.0.0')); | ||
|
|
||
| const release2Link = screen.getByText('Release 1.1.0').closest('a'); |
There was a problem hiding this comment.
Nit: instead of using closest could we use a getByRole to grab the anchor element directly?
| it('does not render when searchEnabled is false', () => { | ||
| render(<SearchComponent searchEnabled={false} />) | ||
|
|
||
| const searchInput = screen.queryByPlaceholderText('Search') |
There was a problem hiding this comment.
Nit: I find it odd that the way we're trying to get the searchInput is so different in this test vs the previous test
| it('updates the search value when user types', () => { | ||
| render(<SearchComponent />) | ||
| waitFor(() => { | ||
| const searchInput = screen.getByPlaceholderText('Search') |
There was a problem hiding this comment.
Also could we use a getByRole for these queries?
| export const updateThemePreference = (theme: Theme) => { | ||
| if(typeof window !== undefined) { | ||
| window?.localStorage?.setItem('theme-preference', theme) | ||
| document?.querySelector('html')?.classList.toggle('pf-v6-theme-dark', theme === 'dark' ); | ||
| }; | ||
| }; | ||
| export const getThemePreference = (): Theme => { | ||
| const theme = window?.localStorage?.getItem ('theme-preference'); | ||
| return theme !== undefined ? (theme as Theme) : 'system'; | ||
| }; | ||
|
|
||
| export const themeLoader = () => { | ||
| if(typeof window !== 'undefined') { | ||
| window.addEventListener('DOMContentLoaded', ()=> { | ||
| updateThemePreference(getThemePreference()); | ||
| }) | ||
| }; | ||
| } No newline at end of file |
There was a problem hiding this comment.
Can we add tests for these utils?
| </ToolbarItem> | ||
| </ToolbarContent> | ||
| </PFToolbar> | ||
| ) |
There was a problem hiding this comment.
Can we add tests for the Toolbar component itself?
This closes issue patternfly/patternfly-doc-core#9. Hardens the page template code, and adds unit testing.