(fix) Make ConfigurableLink call onBeforeNavigate for alternative clicks #884
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
brandones
changed the title
|
Size Change: +346 B (0%) Total Size: 2.84 MB
ℹ️ View Unchanged
|
denniskigen
approved these changes
Jan 16, 2024
There was a problem hiding this comment.
LGTM! Thanks, @brandones. One of the places in the app where alternative clicks are used is the Admin tools app. Here's a video of it in action (for posterity):
alternative-clicking-behaviour.mp4
I'm torn about whether the default behaviour should be to CTRL-click to a separate tab. On the one hand, the things getting linked to here are arguably non-core EMR features, so you can kind of see the rationale for that. On the other hand, that the navigation pattern isn't consistent feels a bit incongruous to me.
|
@denniskigen alternative clicks? Isn't it just that in the admin app, normal clicks open new tabs? That would be unrelated to this change. Where does the admin page code live? I couldn't find it. What I'm wondering is how those links work, are they passing |
|
Yes, they are. Which is why they open in a new tab. This is in the admin tools app. And yeah, it's unrelated to what you've worked on here, I just wanted to get your opinion on whether you think its appropriate for those links to open in a new tab. |
|
Nice, thanks for the link! Hmmm actually it looks like they are using a I do think this is a reasonable thing to do. It might also be good to at some point ensure that |
|
Cool, thanks for the sanity check! And yes, it would be nice to have that configurability. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Requirements
feat,fix, orchore, among others). See existing PR titles for inspiration.For changes to apps
If applicable
Summary
It was the case that
onBeforeNavigatewouldn't be called when the user ctrl-clicks something. Since ctrl-click still navigates the user to the target page, albeit in a different tab, it seems like the app should probably behave in the same way as if the user had clicked the link normally. I'm not aware of any bugs that this fixes, but it seems like it should produce more consistent behavior.