-
-
Notifications
You must be signed in to change notification settings - Fork 719
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 navigation behavior on enterprise settings panel tabs #11799
Fix navigation behavior on enterprise settings panel tabs #11799
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice one. Thank you!
@@ -37,6 +37,16 @@ export default class extends Controller { | |||
window.addEventListener("tabs-and-panels:click", (event) => { | |||
this.simulateClick(event.detail.tab, event.detail.panel); | |||
}); | |||
|
|||
window.addEventListener("popstate", (event) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TIL about popstate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Worth noting that this doesn't fire only on user navigation actions, but also any programmatic changes to history entries, such as history.pushState()
. That's why I have to compare the panels to make sure we're responding to a new tab change. Other than that, popstate
is a really useful event for solving this!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice one 👍
Hey @murjax , This is looking great. I've tried to sum it up in a video, as a quick demonstration: the above section displays a staging server in which the PR was not staged; below, we see a section from a server in which this PR was staged: https://www.loom.com/share/1e683d593f0b4087a9d04a6706861929?sid=ed2122ae-fc6c-4482-9ab1-adbb91fc61d8 This makes it possible to navigate back to sections which were changed, but not saved. Thanks! |
… spec where a modal does not need acceptance Tests forward navigation
…igation Moves test introduced by #11799 to avoid non-explicit modal acceptance
… spec where a modal does not need acceptance Tests forward navigation
What? Why?
Panel navigation through back and forward buttons isn't working on enterprise settings page. The tab selection changes, but the content does not change.
What should we test?
admin/enterprises/[ENTERPRISE-SLUG]/edit#/primary_details
).Other: Make changes to form fields. Back/Forward navigation should show the "unsaved changes" alert.
Screencast.from.11-10-2023.01.13.42.PM.webm
Release notes
Changelog Category (reviewers may add a label for the release notes):
The title of the pull request will be included in the release notes.
Dependencies
Documentation updates