Skip to content

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

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Oct 9, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
55f4125
feature: provide an API to make router ignore an anchor
mstahv Sep 29, 2023
430f341
Apparently some verificaiton tests insist to have a getter as well...
mstahv Sep 29, 2023
80fb78a
fix test to handle new property with different attribute name
caalador Oct 5, 2023
8796fd0
Merge branch 'main' into feature/router-ignore
caalador Oct 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 23 additions & 1 deletion flow-html-components/src/main/java/com/vaadin/flow/component/html/Anchor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,10 +170,32 @@ public void removeHref() {
*/
public void setHref(AbstractStreamResource href) {
this.href = href;
getElement().setAttribute(ROUTER_IGNORE_ATTRIBUTE, true);
setRouterIgnore(true);
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 •
edited
Loading

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 •
edited
Loading

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 😬

* anchor and this way make this anchor behave normally and cause a full
* page load.
*
* @param ignore
* true if this link should not be intercepted by the single-page
* web application routing mechanism in Vaadin.
*/
public void setRouterIgnore(boolean ignore) {
getElement().setAttribute(ROUTER_IGNORE_ATTRIBUTE, ignore);
}

/**
* @return true if this anchor should be ignored by the Vaadin router and
* behave normally.
*/
public boolean isRouterIgnore() {
return getElement().hasAttribute(ROUTER_IGNORE_ATTRIBUTE);
}

/**
* Gets the URL that this anchor links to.
*
Expand Down
2 changes: 2 additions & 0 deletions flow-html-components/src/test/java/com/vaadin/flow/component/html/AnchorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,8 @@ public void getTargetValue_useSomeStringValue_targetIsReturned() {
protected void addProperties() {
addStringProperty("href", "", false);
addOptionalStringProperty("target");
addProperty("routerIgnore", boolean.class, false, true, false, true,
"router-ignore");
}

@Test
Expand Down
6 changes: 6 additions & 0 deletions flow-html-components/src/test/java/com/vaadin/flow/component/html/ComponentProperty.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@

public class ComponentProperty {
public String name;
public String propertyOrAttributeTag;
public Object defaultValue, otherValue;
public boolean optional;
public boolean removeDefault;
Expand All @@ -34,13 +35,18 @@ public <T> ComponentProperty(Class<? extends Component> componentType,
boolean optional, boolean removeDefault) {
this.componentType = componentType;
this.name = name;
this.propertyOrAttributeTag = name;
this.type = type;
this.defaultValue = defaultValue;
this.optional = optional;
this.removeDefault = removeDefault;
this.otherValue = otherValue == null ? name + name : otherValue;
}

public void setPropertyOrAttributeTag(String propertyOrAttributeTag) {
this.propertyOrAttributeTag = propertyOrAttributeTag;
}

public boolean isOptional() {
return optional;
}
Expand Down
13 changes: 12 additions & 1 deletion flow-html-components/src/test/java/com/vaadin/flow/component/html/ComponentTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,16 @@ protected <U> void addProperty(String propertyName, Class<U> propertyType,
isOptional, removeDefault));
}

protected <U> void addProperty(String propertyName, Class<U> propertyType,
U defaultValue, U otherValue, boolean isOptional,
boolean removeDefault, String propertyOrAttributeTag) {
ComponentProperty property = new ComponentProperty(
getComponent().getClass(), propertyName, propertyType,
defaultValue, otherValue, isOptional, removeDefault);
property.setPropertyOrAttributeTag(propertyOrAttributeTag);
properties.add(property);
}

protected Component createComponent() throws InstantiationException,
IllegalAccessException, ClassNotFoundException {
String componentClass = getClass().getName().replace("Test", "");
Expand Down Expand Up @@ -370,7 +380,8 @@ private void assertNonDefaultValues() throws Exception {
Assert.assertEquals(property.otherValue,
property.getUsingGetter(component));
}
assertPropertyOrAttribute(component, property.name);
assertPropertyOrAttribute(component,
property.propertyOrAttributeTag);
}
}

Expand Down