Component mega menu #139
Component mega menu #139
Conversation
|
Mobile menu is still in progress. |
| import styles from './image-teaser.module.css'; | ||
|
|
||
| interface ImageTeaserProps extends GessoComponent { | ||
| export interface ImageTeaserProps extends GessoComponent { |
There was a problem hiding this comment.
Please put the export at the bottom of the file. Eventually we'll lint for this (there's an open issue on this repo), but for now, we at least want to make sure we follow that convention with any new code added.
| document.removeEventListener('click', handleClickAnywhere); | ||
| document.removeEventListener('keydown', handleKeydownAnywhere); | ||
| }; | ||
| }); |
There was a problem hiding this comment.
You're missing your dependency array for useEffect.
| } | ||
|
|
||
| topLevelItems.current.forEach(button => { | ||
| toggleSection(button.nextElementSibling as HTMLDivElement, true); |
There was a problem hiding this comment.
If it's important that the button's next sibling be a div, we should probably do a check rather than forcing it. A type predicate function might be useful here.
| }; | ||
| }); | ||
|
|
||
| const toggleSection = (section: HTMLDivElement, hide: boolean) => { |
There was a problem hiding this comment.
Since you're using this inside your useEffect, it either needs to be inside the useEffect function or created with useCallback().
| }); | ||
|
|
||
| const toggleSection = (section: HTMLDivElement, hide: boolean) => { | ||
| const button = section.previousElementSibling as HTMLButtonElement; |
There was a problem hiding this comment.
Same note as above. If we need the previous element sibling to be type HTMLButtonElement, we should check for that.
| left: 0; | ||
| position: absolute; | ||
| top: 0; | ||
| width: 100%; |
There was a problem hiding this comment.
Please use logical properties for the size and positioning
| .megaMenu > & { | ||
| border-bottom: 1px solid var(--grayscale-white); | ||
| list-style-type: none; | ||
| padding: 1.25rem 1.8125rem 1.25rem 0.8125rem; |
There was a problem hiding this comment.
Logical properties for the border and padding
| text-align: left; | ||
| transition: all 0.4s ease; | ||
| width: auto; | ||
| z-index: calc(1200 + 1); |
There was a problem hiding this comment.
Same here. You get the picture -- please take another pass through the CSS and ensure you're using logical properties rather than left/right/top/bottom in all cases.
| z-index: 0; | ||
|
|
||
| /* stylelint-disable no-descending-specificity */ | ||
| a, |
There was a problem hiding this comment.
The no-descending-specificity ones sometimes result in nothing making Stylelint happy so we have to disable it. If that's the case here, please include a comment about why it's necessary to turn off the linter. Obviously, if it is possible to adjust the ordering to fix the linting issue, do that instead.
| Safari 14 doesn't recognize :focus-visible, so | ||
| combining with the above selector causes the entire | ||
| styles, including :hover, to be ignored. | ||
| */ |
There was a problem hiding this comment.
We're on Safari 17 now, so no reason to keep doing this. Go ahead and combine the selectors and take out the comment.
2 participants
#136 should be merged first.