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

feat(v2): add and use NavigationTabs template components for admin form tab navigation #4601

Merged
merged 12 commits into from
Aug 25, 2022

Conversation

karrui
Copy link
Contributor

@karrui karrui commented Aug 18, 2022

Problem

This PR updates the components used for navigation from ChakraUI tabs to a faux tab component meant to be used for links.
This component fixes a bug where the blocking of navigation would fail (infinite loop) until user accepts the navigation block. That was due to ChakraUI's Tab component retriggering its onChange callback continuously (which retriggers navigation, which retriggers the block).

By using real links instead of the Tab component, no react-router-dom#navigate call is called and users can safely accept/reject the prompt.

Related to #4144

Solution

Breaking Changes

  • No - this PR is backwards compatible

Features:

  • add and use NavigationTabs template components for admin form tab navigation

Before & After Screenshots

Should have absolutely no visual changes

karrui added 6 commits August 18, 2022 13:43
Usage of Tab component was messing with the navigation prompt, since the Tab component's tab index onChange callback will fire continuously even if user dismisses the prompt.

This was a really hard bug to drill down to...
so the NavigationTab component has full height
@karrui karrui changed the title feat(vForm v2/navigation tabs template feat(v2): add NavigationTabs template components Aug 18, 2022
@karrui karrui changed the title feat(v2): add NavigationTabs template components feat(v2): add and use NavigationTabs template components for admin form tab navigation Aug 18, 2022
Copy link
Contributor

@justynoh justynoh left a comment

Choose a reason for hiding this comment

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

lgtm, just a minor comment

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.

2 participants