Skip to content

Conversation

@yuberdysheva
Copy link
Contributor

@yuberdysheva yuberdysheva commented Jun 1, 2023

@gravity-ui-bot
Copy link
Contributor

Preview is ready.

@yuberdysheva yuberdysheva force-pushed the update-header-and-navigation branch from 8c43b12 to d6bf7bf Compare June 9, 2023 09:59
@yuberdysheva yuberdysheva requested a review from aeksandla June 9, 2023 10:19
}
};

window.addEventListener('scroll', _.debounce(handleScroll, 5), {passive: true});
Copy link
Contributor

Choose a reason for hiding this comment

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

mb set time to 100?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if I set 100 there is big delay on scroll before border shows up. I set 20

}, []);

useEffect(() => {
const handleScroll = () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

seems like we should add this handler only if !withBorder is true

const {leftItems, rightItems, iconSize = 20, withBorder = false} = data;
const [isSidebarOpened, setIsSidebarOpened] = useState(false);
const [activeItemId, setactiveItemId] = useState<string | undefined>(undefined);
const [withHeaderBorder, setWithHeaderBorder] = useState(withBorder);
Copy link
Contributor

@gorgeousvlad gorgeousvlad Jun 9, 2023

Choose a reason for hiding this comment

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

what is the difference between withBorder and withHeaderBorder?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • We pass the prop withBorder which can show or hide border when scrollY = 0.
  • We always have to show border when scrollY > 0.
  • I chose not to rewrite the original prop to hide/show border but create a new one withHeaderBorder and change this state if it's necessary. I rename variable withHeaderBorder to showBorder is that better?

useEffect(() => {
const handleScroll = () => {
if (window.scrollY > 0 && !withBorder) {
setWithHeaderBorder(true);
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we set withHeaderBorder when withBorder is false?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because withBorder = false needs only for scrollY = 0. If we have scrollY > 0 we have to show border all the time

Copy link
Contributor

Choose a reason for hiding this comment

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

If we have scrollY > 0 we have to show border all the time
then should this condition be like if(window.scrollY > 0)?

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's not necessary. If withBorder = true we don't have to change anything.
const [showBorder, setShowBorder] = useState(withBorder);
showBorder is constantly true

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 we can check showBorder instead of withBorder if you think so

aeksandla
aeksandla previously approved these changes Jun 9, 2023
gorgeousvlad
gorgeousvlad previously approved these changes Jun 9, 2023
@yuberdysheva yuberdysheva merged commit 7757ec1 into main Jun 13, 2023
@yuberdysheva yuberdysheva deleted the update-header-and-navigation branch June 13, 2023 11:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants