Skip to content
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

feat: navigation dropdown refactoring #392

Merged
merged 3 commits into from
Jul 25, 2023
Merged

feat: navigation dropdown refactoring #392

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

Preview is ready.

@NikitaCG NikitaCG self-requested a review June 8, 2023 14:47
@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
src/hooks/useGetNavigationItem.ts Outdated Show resolved Hide resolved
src/navigation/components/DesktopNavigation/DesktopNavigation.tsx Outdated Show resolved Hide resolved
src/navigation/components/DesktopNavigation/DesktopNavigation.tsx Outdated Show resolved Hide resolved
...on/components/DesktopNavigation/components/RightDesktopNavigation/RightDesktopNavigation.tsx Outdated Show resolved Hide resolved
...igation/components/MobileNavigation/components/MobileNavigationItem/MobileNavigationItem.tsx Outdated Show resolved Hide resolved
src/navigation/components/Navigation/Navigation.tsx Outdated Show resolved Hide resolved
src/navigation/components/Navigation/Navigation.tsx Outdated Show resolved Hide resolved
src/navigation/components/NavigationItem/NavigationItem.tsx Outdated Show resolved Hide resolved
gorgeousvlad
gorgeousvlad requested 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
Collaborator

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

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
&__wrapper {
display: flex;
justify-content: space-between;
align-items: center;

height: var(--header-height);

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

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

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
Collaborator

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

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
Collaborator

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

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
Collaborator

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

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
Collaborator

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

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
@niktverd niktverd force-pushed the navigation_dropdown_refactoring branch from 5256f27 to 974932a Compare July 21, 2023 07:54
@yuberdysheva
Copy link
Contributor

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 requested changes Jul 24, 2023
View reviewed changes
src/navigation/components/MobileMenuButton/MobileMenuButton.tsx Outdated
isSidebarOpened,
onSidebarOpenedChange,
}) => {
const iconProps = {
Copy link
Collaborator

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

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
Collaborator

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

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
Collaborator

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

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
Collaborator

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

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
3 checks passed
@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

@gorgeousvlad gorgeousvlad gorgeousvlad approved these changes

@yuberdysheva yuberdysheva yuberdysheva left review comments

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

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@niktverd @gravity-ui-bot @yuberdysheva @gorgeousvlad