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.

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

fix(HeaderContainer): use previous state #7890

Merged

Conversation

kalbert312
Copy link
Contributor

I noticed specifically on Safari on iPhone that sometimes tapping the side nav button has no effect in our app, at least on the first try.
Not sure what's causing it but taking a look at the code I believe the recommended way to update state if it depends on previous status value is to use the callback variant. My theory is that somehow our code is getting a stale reference for onClickSideNavExpand in some cases. In any case this should prevent any such issue from occurring.

On a related note I needed to do the following to handle external link clicks and closing the side nav:

import { useUpdate } from "react-use";
...
const rerender = useUpdate();
...
<SideNavLink<React.AnchorHTMLAttributes<HTMLAnchorElement>>
  href={href}
  onClick={(e) => {
    e.preventDefault();
    onClickSideNavExpand();
    rerender(); // this is required otherwise the side nav won't close
    window.open(href, "_blank", "noopener,noreferrer");
  }}
>
  Some external link
</SideNavLink>

I'm thinking it might be related to this button click issue somehow.

Changelog

Changed

  • Update HeaderContainer to use previous state when changing the expanded state.

@netlify
Copy link

netlify bot commented Feb 24, 2021

Deploy preview for carbon-elements ready!

Built with commit 360d51b

https://deploy-preview-7890--carbon-elements.netlify.app

@netlify
Copy link

netlify bot commented Feb 24, 2021

Deploy preview for carbon-components-react ready!

Built without sensitive environment variables with commit 360d51b

https://deploy-preview-7890--carbon-components-react.netlify.app

@kalbert312
Copy link
Contributor Author

kalbert312 commented Feb 24, 2021

After some more testing... it might be caused by slight finger movement that may be registering as a scroll and not a click. When it does happen it looks like the side nav button gets the focus/active highlight but does not open. I think this change is good to have anyway.

Copy link
Collaborator

@tw15egan tw15egan left a comment

Choose a reason for hiding this comment

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

LGTM 👍 ✅

@kodiakhq kodiakhq bot merged commit 8322ccd into carbon-design-system:master Feb 24, 2021
@dakahn dakahn mentioned this pull request Mar 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants