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

Allow ListItem components to link to external links #3040

Merged
merged 1 commit into from
Aug 18, 2022

Conversation

tcitworld
Copy link
Contributor

Closes #3039

I didn't do anything UI-wise to distinguish external links (which use target="_blank", but we could change that).

@tcitworld tcitworld added the 3. to review Waiting for reviews label Aug 15, 2022
@tcitworld tcitworld requested review from marcoambrosini, jotoeri, a team, juliusknorr and CarlSchwan and removed request for a team August 15, 2022 15:28
@tcitworld tcitworld force-pushed the allow-listitems-to-have-external-links branch from e9b4dc2 to 5d4d7b6 Compare August 18, 2022 07:40
@tcitworld tcitworld force-pushed the allow-listitems-to-have-external-links branch from 5d4d7b6 to 5ba557d Compare August 18, 2022 08:44
@artonge artonge merged commit 28a1be5 into master Aug 18, 2022
@artonge artonge deleted the allow-listitems-to-have-external-links branch August 18, 2022 10:36
@skjnldsv skjnldsv mentioned this pull request Aug 18, 2022
Comment on lines +195 to +196
target="_blank"
rel="noopener noreferrer"
Copy link
Contributor

Choose a reason for hiding this comment

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

These two now always apply! This breaks the component.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shouldn't it be harmless on a <a href="#"> tag? How does it break it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, we use @click for it, no?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, but the target blank seems to kick in before click is emitted. Thus the appnavigation on forms now opens a new tab with the link instead of triggering the router.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#3069 should do it. Click event would have a different behaviour if e.preventDefault() wouldn't be used.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow listitem components to have external links
4 participants