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

feature: provide an API to make router ignore an anchor #17745

Merged
merged 4 commits into from
Oct 9, 2023

Conversation

mstahv
Copy link
Member

@mstahv mstahv commented Sep 29, 2023

Description

Anchors in Vaadin are simply broken if the url happens to be relative. With this API that can be fixed.

Fixes #17617

Type of change

  • Bugfix
  • Feature

Checklist

  • I have read the contribution guide: https://vaadin.com/docs/latest/guide/contributing/overview/
  • I have added a description following the guideline.
  • The issue is created in the corresponding repository and I have referenced it.
  • I have added tests to ensure my change is effective and works as intended.
  • New and existing tests are passing locally with my change.
  • I have performed self-review and corrected misspellings.

Additional for Feature type of change

  • Enhancement / new feature was discussed in a corresponding GitHub issue and Acceptance Criteria were created.

@mcollovati mcollovati added the Contribution PRs coming from the community or external to the team label Sep 29, 2023
@github-actions
Copy link

github-actions bot commented Sep 29, 2023

Test Results

1 007 files  ±0  1 007 suites  ±0   1h 4m 24s ⏱️ +33s
6 428 tests ±0  6 387 ✔️ ±0  41 💤 ±0  0 ±0 
6 686 runs   - 1  6 638 ✔️  - 1  48 💤 ±0  0 ±0 

Results for commit 8796fd0. ± Comparison against base commit ad75057.

♻️ This comment has been updated with latest results.

assignHrefAttribute();
}

/**
* The routing mechanism in Vaadin by default intercepts all anchor elements
* with relative URL. This method can be used make the router ignore this
Copy link
Contributor

@knoobie knoobie Sep 29, 2023

Choose a reason for hiding this comment

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

I'm wondering if it would be worth to add e.g. a info-level logging in development mode on attach (like done in Label) to inform the user about this behavior when setRouterIgnore(false) and setHref(relative path)?

I feel like most of the "pitfalls" (when they can't be changed easily, cause "senior" users are used to it by now and would unnessary result in breaking changes) can be improved for new user by providing such information in the logs to notify them about the problem and possible better practice?

Edit: Totally out of scope, just thinking out loud: Instead of bloating all components.. the devserver could contain hooks that are always called when components are added to the layout and could implement such checks based on a "ruleset", allowing to improve UX, DX and probably even the whole application overall by providing helpfull tipps like:

  • "Anchor with relatives paths and no router-link attribute are handled by Vaadin. Is this intended?"
  • "Found foo in bar. It would be better to use baz instead to improve accessibility."
  • ...

Sell it as pro-code 😉

Copy link
Member Author

Choose a reason for hiding this comment

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

More than the added phase (which might be handy as well), would be more/as handy if could intercept the phase when the state is about to be synced with the client. That I have missed a dozen of times. There is this related UI.beforeClientResponse, but that is also component specific (and the method is in weird place and not that well documented in our reference docs).

https://github.com/vaadin/flow/blob/main/flow-server/src/main/java/com/vaadin/flow/component/UI.java#L1283-L1332

Copy link
Member Author

@mstahv mstahv Oct 5, 2023

Choose a reason for hiding this comment

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

And yeah, something derived of that could be used for some actual logic as well, not just complaining that our users have not figured out that we have bad defaults ;-)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah "beforeClientResponse".. I remember that method, everytime I see it in the flow components it feels like some hack-ish way to workaround limitations 😬

@sonarcloud
Copy link

sonarcloud bot commented Oct 5, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 7 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

Copy link
Member

@tltv tltv left a comment

Choose a reason for hiding this comment

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

Adding some info logging in dev mode as suggested would be nice, even if it's done in same way as in the Label. Approved as the API looks good as is.

@mstahv
Copy link
Member Author

mstahv commented Oct 5, 2023

Not planning to add that. It would only help a tiny bit in the big picture. As discussed in the ticket a better solution would be to the change the default in Anchor component. But still, most broken links probably come via other links than those created with Anchor (like richtextarea editing html coming to be displayed within same app) and the true fix would be to admit that we have had this wrong default since V15 in the client side router itself. That should be changed and the only thing that would break are some rare Vaadin-Hilla integrations - otherwise changing the default would only fix current apps (hunch, haven't checked if people (mis)use Anchor for in-app navigation).

@knoobie
Copy link
Contributor

knoobie commented Oct 5, 2023

I agree with Matti (it was just a thought experiment), but I trust people to missuse the anchor, like any other component 🙈

@mshabarov mshabarov merged commit fe84828 into main Oct 9, 2023
26 checks passed
@mshabarov mshabarov deleted the feature/router-ignore branch October 9, 2023 09:59
@vaadin-bot
Copy link
Collaborator

This ticket/PR has been released with Vaadin 24.3.0.alpha1 and is also targeting the upcoming stable 24.3.0 version.

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

Successfully merging this pull request may close these issues.

Provide method to set Anchor's router-ignore flag.
7 participants