-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
LinkTo Transition Bug #16983
LinkTo Transition Bug #16983
Conversation
&.active { | ||
&.is-active { |
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.
While attempting to test that the nav drawer closed when clicking a link on smaller screens I noticed that it wasn't appearing on screen in the first place. Updating the class name to match the template fixed the issue.
Deployment failed with the following error:
|
@models={{this.link.models}} | ||
@query={{this.query}} | ||
@disabled={{@disabled}} | ||
{{on "click" this.onLinkClick}} |
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.
This was only being used in the SecretVersionMenu
component to close the menu but since a new route is transitioned to the menu is torn down making it unnecessary.
@@ -113,7 +113,6 @@ | |||
"ember-export-application-global": "^2.0.1", | |||
"ember-fetch": "^8.1.1", | |||
"ember-inflector": "4.0.2", | |||
"ember-link-action": "^1.0.0", |
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.
This was no longer used as of 1.11
} | ||
}, | ||
}; | ||
}; |
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.
This probably won't be used again but it might be good to keep all of the recast scripts around for examples to help with future codemods.
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.
This is great to see, as someone who's never written a codemod!
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.
I wonder if we could keep leveraging LinkTo
but use a different method to trigger callbacks?
ui/app/templates/vault/cluster.hbs
Outdated
@@ -12,48 +12,52 @@ | |||
<NamespacePicker @class="navbar-item" @namespace={{this.namespaceQueryParam}} /> | |||
</li> | |||
{{/if}} | |||
<li class={{if (is-active-route "vault.cluster.secrets") "is-active"}}> | |||
<li class={{if (is-active-route "vault.cluster.secrets") "is-active"}} role="button" {{on "click" Nav.closeDrawer}}> |
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.
I think I like this pattern better, where we add the "side effects" click event on the event outside the element, which just has the @route
link. I'm concerned that making these links technically buttons violates some A11y concerns, except of course in cases when the button is triggering a save or submit, and then redirecting.
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.
I wondered about that too and ultimately followed the suggestions from the linter. I think since the li
is the parent element it's not actually the link so the role of button might be ok? Basically the li is acting as a button to close the nav drawer. The nested link functions as a link to transition routes.
} | ||
}, | ||
}; | ||
}; |
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.
This is great to see, as someone who's never written a codemod!
I had some success using |
@action | ||
closeDropdown(dropdown) { | ||
// strange issue where closing dropdown triggers full transition which redirects to auth screen in production builds | ||
// closing dropdown in next tick of run loop fixes it |
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.
🤯
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.
Haha yes! I noticed that a full page reload was happening when only a partial transition should have been taking place. I wonder if closing the dropdown in the same tick was throwing an error behind the scenes which caused issues with the transition?
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.
Thank you for your diligence! 🎉
It was discovered that in production builds of Ember, LinkTo elements that used the on click modifier were transitioning to the incorrect route. During the latest Ember upgrade a new lint rule was alerting of unsupported arguments being used on the LinkTo component. In this case,
invokeAction
was being provided by theember-link-action
addon which was removed in favor of using theon
modifier.Since it works as expected in development builds it seems like it could be a bug in Ember, but it also seems like a link that has a click event is maybe not the best pattern. After attempting to find a way to make a click event work it was decided to use the
button
element instead along with the router service to transition inside the event handler.Migrations Details
A script was created for
ember-template-recast
to identify any template files that hadLinkTo
elements with theon
modifier. The UI elements impacted were:Where feasible, the
LinkTo
elements were changed to buttons and the router service was used to transition. In other cases where a link was appropriate theon
modifier was added to the parent element.Update
In the interests of accessibility the updates to use the
button
element overLinkTo
were reverted. After further testing, the issue was not with the on click modifier but specifically with closing the dropdown in the event handler. To work around this the dropdown is now being closed in the next tick of the run loop which allows for the transition to complete as expected.