Skip to content

feat: navigation dropdown refactoring #392

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
niktverd merged 3 commits into from
Jul 25, 2023
Merged

feat: navigation dropdown refactoring #392

niktverd merged 3 commits into from
Jul 25, 2023

Conversation

@niktverd
Copy link
Contributor

@niktverd niktverd commented Jun 8, 2023

No description provided.

@gravity-ui-bot
Copy link
Contributor

gravity-ui-bot commented Jun 8, 2023

Preview is ready.

@NikitaCG NikitaCG self-requested a review June 8, 2023 14:47
yuberdysheva
yuberdysheva suggested changes Jun 8, 2023
View reviewed changes
@niktverd niktverd force-pushed the navigation_dropdown_refactoring branch from 7a3bacc to 79dc2bc Compare July 4, 2023 17:15
yuberdysheva
yuberdysheva suggested changes Jul 6, 2023
View reviewed changes
gorgeousvlad
gorgeousvlad suggested changes Jul 6, 2023
View reviewed changes
src/hooks/useGetNavigationItem.ts Outdated

const iconSizeKey: keyof NavigationItemBase = 'iconSize';

export default function useGetNavigationItem(iconSize = 20) {
Copy link
Contributor

@gorgeousvlad gorgeousvlad Jul 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why it is not just a getNavigationItemWithIconSize function? It doesn't use any hooks inside(useCallback can be removed).

Copy link
Contributor Author

@niktverd niktverd Jul 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

src/navigation/components/DesktopNavigation/DesktopNavigation.scss Outdated

height: var(--header-height);

margin-block-start: 0px;
Copy link
Contributor

@gorgeousvlad gorgeousvlad Jul 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we decide to support indents like here instead of just margins?

Copy link
Contributor Author

@niktverd niktverd Jul 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it was a rudiment

src/navigation/components/DesktopNavigation/DesktopNavigation.tsx
<div className={b('wrapper')}>
{logo && (
<div className={b('left')}>
<Logo {...logo} className={b('logo')} />
Copy link
Contributor

@gorgeousvlad gorgeousvlad Jul 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should Logo be inside of LeftDesktopNavigation and MobileMenuButton is inside of RightDesktopNavigation?

Copy link
Contributor Author

@niktverd niktverd Jul 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think "No, they should not". Both Logo and MobileMenuButton are specific items: they are always visible on every screen, they are not a part of concept NavigationItem, adding this to Left and Right components will make logic of ones complicated

...tion/components/DesktopNavigation/components/LeftDesktopNavigation/LeftDesktopNavigation.tsx Outdated
@@ -0,0 +1,71 @@
import React, {useCallback, useContext, useEffect} from 'react';
Copy link
Contributor

@gorgeousvlad gorgeousvlad Jul 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

src/navigation/components/DesktopNavigation/components/Left/Left.tsx?

Copy link
Contributor Author

@niktverd niktverd Jul 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agree

...tion/components/DesktopNavigation/components/LeftDesktopNavigation/LeftDesktopNavigation.tsx Outdated
const {asPath, pathname} = useContext(LocationContext);

const hidePopup = useCallback(() => {
onActiveItemChange();
Copy link
Contributor

@gorgeousvlad gorgeousvlad Jul 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems like we can use onActiveItemChange instead of hidePopup

Copy link
Contributor Author

@niktverd niktverd Jul 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure

src/navigation/components/Navigation/Navigation.tsx Outdated
useEffect(() => {
hidePopup();
}, [hidePopup, asPath, pathname]);
const onSidebarOpenedChange = useCallback((isOpen: boolean) => {
Copy link
Contributor

@gorgeousvlad gorgeousvlad Jul 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lets remove useCallback

Copy link
Contributor Author

@niktverd niktverd Jul 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure

@niktverd niktverd force-pushed the navigation_dropdown_refactoring branch from 2f609a9 to f13393d Compare July 20, 2023 06:15
yuberdysheva
yuberdysheva suggested changes Jul 20, 2023
View reviewed changes
@niktverd niktverd force-pushed the navigation_dropdown_refactoring branch from 5256f27 to 974932a Compare July 21, 2023 07:54
@yuberdysheva
Copy link
Contributor

yuberdysheva commented Jul 24, 2023

I have one more question about navigation dropdown with highlight
image
Let's check it with design

yuberdysheva
yuberdysheva previously approved these changes Jul 24, 2023
View reviewed changes
gorgeousvlad
gorgeousvlad suggested changes Jul 24, 2023
View reviewed changes
src/navigation/components/MobileMenuButton/MobileMenuButton.tsx Outdated
isSidebarOpened,
onSidebarOpenedChange,
}) => {
const iconProps = {
Copy link
Contributor

@gorgeousvlad gorgeousvlad Jul 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this variable is not necessary, you can remove it and also remove return to make notation more compact

Copy link
Contributor Author

@niktverd niktverd Jul 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure

src/navigation/components/Navigation/Navigation.scss Outdated
&__links {
position: relative;
position: sticky;
z-index: 98;
Copy link
Contributor

@gorgeousvlad gorgeousvlad Jul 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it needs to be constant because other way there is the question: 'Why 98'?)

Copy link
Contributor Author

@niktverd niktverd Jul 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why? it is a history and a mystery

page-constructor/src/navigation/components/Header/Header.scss

Line 10 in b449e43

z-index: 98;

made a constant

src/navigation/components/Navigation/Navigation.tsx Outdated
};

window.addEventListener('scroll', _.debounce(showBorderOnScroll, 20), {passive: true});
return () => window.removeEventListener('scroll', _.debounce(showBorderOnScroll, 20));
Copy link
Contributor

@gorgeousvlad gorgeousvlad Jul 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure but I think it should be link to same handler in addEventListener and removeEventListener. Now you are creating different ones with _.debounce

Copy link
Contributor Author

@niktverd niktverd Jul 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point

src/navigation/models.ts Outdated
export interface NavigationListProps
extends Pick<
NavigationListItemProps,
'activeItemId' | 'column' | 'className' | 'onActiveItemChange' | 'menuLayout'
Copy link
Contributor

@gorgeousvlad gorgeousvlad Jul 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there are activeItemId and onActiveItemChange repeating in interface below, could we create some base interface?

Copy link
Contributor Author

@niktverd niktverd Jul 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@niktverd niktverd merged commit 9f47dbe into main Jul 25, 2023
@niktverd niktverd deleted the navigation_dropdown_refactoring branch July 25, 2023 02:15
@gravity-ui-bot gravity-ui-bot mentioned this pull request Jul 25, 2023
Merged
Kyzyl-ool pushed a commit that referenced this pull request Jul 27, 2023
@niktverd
feat: navigation dropdown refactoring (#392)
3dd6b2f 
* feat: navigation dropdown refactoring
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@NikitaCG NikitaCG Awaiting requested review from NikitaCG NikitaCG is a code owner

2 more reviewers

@gorgeousvlad gorgeousvlad gorgeousvlad approved these changes

@yuberdysheva yuberdysheva yuberdysheva left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@niktverd @gravity-ui-bot @yuberdysheva @gorgeousvlad