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 useEffect() in sky nav #1918

Merged
merged 3 commits into from
Jul 7, 2022
Merged

Fix useEffect() in sky nav #1918

merged 3 commits into from
Jul 7, 2022

Conversation

calebeby
Copy link
Member

@calebeby calebeby commented Jul 7, 2022

Closes #1909

The useEffect was being called multiple times, where it was before. I don't think it should have been placed where it was, because that is not in the top-level of a component, which is the only place where hooks are allowed.

It seems like Storybook made a change where story components now get rendered multiple times. Because of our placement of useEffect, the sky nav JS gets initialized twice. This caused it to expand and retract immediately when you click the menu button.

I changed the useEffect to be called in a decorator which I think is actually a component, so it should be OK to use useEffect there.

This problem exists in other components too. I can go through them and fix them if there are any others where the multiple-initialization of JS is causing a problem, but I figure that is not a launch blocker since for most of them it seems to not matter.

@tylersticka the sky nav JS in the prototypes that I added in #1503 is broken also, but I think for a different reason. I didn't look into fixing that since I'm not sure if it's important that those continue to work since we've implemented the production version now. Let me know if you want me to fix those!

https://deploy-preview-1918--cloudfour-patterns.netlify.app/?path=/story/components-sky-nav--dark

@calebeby calebeby requested review from tylersticka and a team July 7, 2022 20:15
@changeset-bot
Copy link

changeset-bot bot commented Jul 7, 2022

⚠️ No Changeset found

Latest commit: 4288ba1

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@netlify
Copy link

netlify bot commented Jul 7, 2022

Deploy Preview for cloudfour-patterns ready!

Name Link
🔨 Latest commit 4288ba1
🔍 Latest deploy log https://app.netlify.com/sites/cloudfour-patterns/deploys/62c74532a7b8d40009a532d3
😎 Deploy Preview https://deploy-preview-1918--cloudfour-patterns.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@calebeby calebeby marked this pull request as ready for review July 7, 2022 20:26
@calebeby calebeby requested a review from tylersticka July 7, 2022 20:26
@tylersticka
Copy link
Member

This problem exists in other components too. I can go through them and fix them if there are any others where the multiple-initialization of JS is causing a problem, but I figure that is not a launch blocker since for most of them it seems to not matter.

Can you create an issue for the broader problem?

the sky nav JS in the prototypes that I added in #1503 is broken also, but I think for a different reason. I didn't look into fixing that since I'm not sure if it's important that those continue to work since we've implemented the production version now. Let me know if you want me to fix those!

It's not important for those to continue to work. Maybe we should remove them if we know they're broken?

@calebeby
Copy link
Member Author

calebeby commented Jul 7, 2022

Can you create an issue for the broader problem?

#1919

It's not important for those to continue to work. Maybe we should remove them if we know they're broken?

Remove all the prototypes which have the sky nav? Or remove the sky nav JS from those? I'm fine with either. Should that be in this PR or another?

@tylersticka
Copy link
Member

tylersticka commented Jul 7, 2022

Remove all the prototypes which have the sky nav? Or remove the sky nav JS from those? I'm fine with either. Should that be in this PR or another?

Either one. (Both questions.)

tylersticka
tylersticka previously approved these changes Jul 7, 2022
@calebeby
Copy link
Member Author

calebeby commented Jul 7, 2022

@tylersticka I removed all the prototypes which call useSkyNav

@tylersticka tylersticka mentioned this pull request Jul 7, 2022
3 tasks
@calebeby calebeby merged commit da3c096 into v-next Jul 7, 2022
@calebeby calebeby deleted the fix-sky-nav-useeffect branch July 7, 2022 23:06
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.

Did our Sky Nav JavaScript break at some point?
2 participants