765-fix: Fix styles active menu item #814
Conversation
github-actions
bot
changed the title
|
The arrow's focus state could be improved This isn't due to your changes, but the spacing is inconsistent here |
|
Lighthouse Report:
|
| setDropdownOpen((prev) => !prev); | ||
| } | ||
| }; | ||
|
|
||
| const handleEscKeyPress = (e: KeyboardEvent<HTMLElement>) => { | ||
| if (e.code === 'Escape') { |
There was a problem hiding this comment.
Let's all keys' codes move to constant like KEY_CODES = {ESCAPE: 'Escape', .....} You could add it here src/core/const/index.ts
YulikK
changed the title
|
Lighthouse Report:
|
…ip and courses pages
| width: max-content; | ||
| min-width: 100%; | ||
| // min-width: calc(100% + var(--header-padding)); |
YulikK
changed the title
|
The breadcrumbs on the site are broken. |
There was a problem hiding this comment.
some design inconsistencies, i found so far
- the top shadow should not be visible
- the shadow value overall is not matched the design
Support Usdescription should't contain hoverSupport Usicons size inconsistent with the designSupport Usthere is a gap missing between columnsSupport Ustypography mismatchCoursesthe course text should be vertically aligned to the endCoursesthe gap between rows inconsistent with the designCoursesthe gap between icon and typography inconsistent with the design- in the header
RSicon should be smaller as the design shows
- bunch of other spacing/typography inconsistencies, please pay attention to these
| </div> | ||
| ); | ||
| }; | ||
| export const DropdownWrapper = forwardRef<HTMLDivElement, DropdownWrapperProps>( |
There was a problem hiding this comment.
since we're using react 19, can we get rid of forward ref please
|
|
||
| const onDropdownToggle = () => { | ||
| if (!isDropdownOpen) { | ||
| window.dispatchEvent(new CustomEvent('closeAllDropdowns')); |
There was a problem hiding this comment.
let's wrap 'closeAllDropdowns' event in constant
| className={cx('dropdown-arrow')} | ||
| aria-expanded={isDropdownOpen} | ||
| > | ||
| {label === 'Support Us' |
There was a problem hiding this comment.
let's think what if in the near future we wanted to change Support Us label to something else, in this case we can potentially miss that nav-item is relying on Support Us string to render the icon.
let's add the new prop to nav-item called icon, and delegate passing the icon to consumer component. In this case the nav-item don't need to worry about what icon to render, and for us developers, in case of refactor, there is no way we can miss that the icon could be potentially lost.
| && (pathname.includes(ROUTES.MENTORSHIP) | ||
| ? ( | ||
| <Image | ||
| src={iconBlue} | ||
| alt="Donate-icon" | ||
| width={18} | ||
| height={16} | ||
| aria-hidden="true" | ||
| data-testid="school-item-icon" | ||
| /> | ||
| ) | ||
| : ( | ||
| <Image | ||
| src={iconYellow} | ||
| alt="Donate-icon" | ||
| width={18} | ||
| height={16} | ||
| aria-hidden="true" | ||
| data-testid="school-item-icon" | ||
| /> | ||
| ))} |
There was a problem hiding this comment.
could be simplified to this
const iconSrc = pathname.includes(ROUTES.MENTORSHIP) ? iconBlue : iconYellow
<Image
src={iconSrc}
alt="Donate-icon"
width={18}
height={16}
aria-hidden="true"
data-testid="school-item-icon"
/>
| export function useOutsideClick<T extends HTMLElement>( | ||
| ref: React.RefObject<T | null>, | ||
| callback: () => void, | ||
| when: boolean, |
There was a problem hiding this comment.
What does when mean in our case?
| <button onClick={() => onMenuItemClick('Community')} className={cx('category-link', color)}> | ||
| <span>Community</span> | ||
| <span | ||
| className={cx('dropdown-arrow', { rotate: Boolean(activeDropdowns.has('Community')) })} | ||
| role="button" | ||
| aria-expanded={activeDropdowns.has('Community')} | ||
| > | ||
| <DropdownArrow /> | ||
| </span> | ||
| </button> |
There was a problem hiding this comment.
this button is duplicated several times across the file, let's wrap it in a reusable component
| const isStaticImageData = (icon: StaticImageData | ReactNode): icon is StaticImageData => | ||
| typeof icon === 'object' && icon !== null && 'src' in icon; | ||
|
|
There was a problem hiding this comment.
let's move this to shared/helpers
| && (isStaticImageData(icon) | ||
| ? ( | ||
| <Image | ||
| src={icon} | ||
| alt="" | ||
| width={32} | ||
| height={32} | ||
| aria-hidden="true" | ||
| data-testid="school-item-icon" | ||
| /> | ||
| ) | ||
| : ( | ||
| <span aria-hidden="true" data-testid="school-item-icon"> | ||
| {icon} | ||
| </span> | ||
| ))} |
There was a problem hiding this comment.
can we please, get rid of renderIcon and inside donateOptions in icon field, just place icons src?
| @@ -12,13 +12,31 @@ type SchoolMenuProps = PropsWithChildren & | |||
| HTMLProps<HTMLUListElement> & { | |||
| heading?: string; | |||
| color?: Color; | |||
| layout?: 'columns' | 'single'; | |||
| mobileClass?: 'visible' | 'hidden'; | |||
There was a problem hiding this comment.
IMO this adds extra complexity to consumer component, hence in the end we are getting duplicated code across the app mobileClass={activeDropdowns.has('Mentorship') ? 'visible' : 'hidden'}>
let's keep it as simple as possible and delegate rendering the right class to the component itself
| mobileClass?: 'visible' | 'hidden'; | |
| isVisible?: boolean; |
| style={ | ||
| layout === 'columns' | ||
| ? { gridTemplateRows: `repeat(${Math.ceil(Children.count(children) / 2)}, auto)` } | ||
| : undefined | ||
| } | ||
| > | ||
| {children} | ||
| </ul> |
There was a problem hiding this comment.
we could simplify this logic by applying these styles
&--columns {
grid-auto-flow: column;
grid-template-rows: repeat(2, 48px);
&:has(:nth-child(5)) {
grid-template-rows: repeat(auto-fit, 48px);
}
}
|
Lighthouse Report:
|
|
Lighthouse Report:
|
|
@JuliaAnt, please check the keyboard navigation. It seems like this functionality is no longer working |
dev-data/donate-options.data.ts
Outdated
| export const donateOptions = [ | ||
| { | ||
| id: 1, | ||
| linkLabel: 'Donate now', | ||
| icon: 'openCollective', | ||
| icon: OpenCollectiveIcon, | ||
| href: LINKS.DONATE_OPEN_COLLECTIVE, | ||
| }, | ||
| { | ||
| id: 2, | ||
| linkLabel: 'Subscribe now', | ||
| icon: 'boosty', | ||
| icon: BoostyIcon, | ||
| href: LINKS.DONATE_BOOSTY, | ||
| }, | ||
| ]; | ||
|
|
||
| export const donateOptionsForNavMenu = [ | ||
| { | ||
| id: 1, | ||
| linkLabel: 'Subscribe on Boosty', | ||
| icon: boostyIconMenu, | ||
| href: LINKS.DONATE_BOOSTY, | ||
| }, | ||
| { | ||
| id: 2, | ||
| linkLabel: 'Donate on Opencollective', | ||
| icon: openCollective, | ||
| href: LINKS.DONATE_OPEN_COLLECTIVE, | ||
| }, | ||
| ]; |
There was a problem hiding this comment.
Can we make one object for support info?
{
id: 1,
buttonLinkLabel: 'Donate now',
menuLinkLabel: 'Donate on Opencollective',
buttonIcon: OpenCollectiveIcon,
menuImage: openCollective
href: LINKS.DONATE_OPEN_COLLECTIVE,
}
There was a problem hiding this comment.
we already have icon boosty-icon.svg. Can we use it for menu and for button?
| justify-content: space-between; | ||
|
|
||
| max-width: 470px; | ||
| margin-right: 62px; |
There was a problem hiding this comment.
Can we not use margin and do it with an assistant property gap?
src/widgets/header/header.tsx
Outdated
| import classNames from 'classnames/bind'; | ||
| import { usePathname } from 'next/navigation'; | ||
|
|
||
| import { BurgerMenu } from './ui/burger/burger'; | ||
| import { donateOptionsForNavMenu } from '../../../dev-data/donate-options.data'; |
There was a problem hiding this comment.
Please add donateOptionsForNavMenu to file dev-data/index.ts
And update import:
import { donateOptionsForNavMenu } from 'data';
src/widgets/header/header.tsx
Outdated
| return ( | ||
| <nav className={cx('navbar', color)} data-testid="navigation"> | ||
| <nav className={cx('navbar', 'white')} data-testid="navigation"> |
There was a problem hiding this comment.
if we have only one color for nav panel now, can we remove white-class and add all propertis in navbar?
| @@ -21,7 +29,7 @@ | |||
| &:hover { | |||
| .title { | |||
| &.dark { | |||
| color: $color-black; | |||
| color: #969fa8; | |||
There was a problem hiding this comment.
lets make a color constant
| opacity: 0; | ||
| } | ||
|
|
||
| &.visible { | ||
| @extend %transition-all; | ||
|
|
||
| max-height: 624px; | ||
| margin-top: 26px; | ||
| visibility: visible; | ||
| opacity: 1; |
There was a problem hiding this comment.
please use constant for opacity
$opacity-100, $opacity-0
| visible: isVisible === true, | ||
| hidden: isVisible === false, |
There was a problem hiding this comment.
can we move all properties from .visible to .school-menu and add .hidden if isVisible===true?
| ))} | ||
| </SchoolMenu> | ||
| </div> | ||
| </div> | ||
| </nav> |
There was a problem hiding this comment.
as a suggestion above, if you're going to split the main header menu into separate blocks, you should split the mobile menu as well. It came out as a very large component
| <Image | ||
| src={icon} | ||
| alt="Donate-icon" | ||
| width={20} | ||
| height={18} | ||
| aria-hidden="true" | ||
| /> |
There was a problem hiding this comment.
I understand that we have only one icon now, but still it seems wrong to set in alt donate-icon - if the icon comes in props, maybe then together with the icon to pass the alt? And let's write “little heart” in this icon alt.
|
Lighthouse Report:
|
|
Lighthouse Report:
|
What type of PR is this? (select all that apply)
Description
Rework desktop and mobile menus according to the new design.
Related Tickets & Documents
Screenshots, Recordings
Before:
After:
Added/updated tests?
[optional] Are there any post deployment tasks we need to perform?
[optional] What gif best describes this PR or how it makes you feel?
Feel free to hit me with your best practices :))