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

feat: add target API to SideNavItem #5962

Merged
merged 4 commits into from
Jan 22, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -357,6 +357,67 @@ public void setPathAliases(Set<String> pathAliases) {
}
}

/**
* Gets the target of this item.
*
* @return the target of this item
*/
public String getTarget() {
return getElement().getProperty("target");
}

/**
* Where to display the linked URL, as the name for a browsing context.
* <p>
* The following keywords have special meanings for where to load the URL:
* <ul>
* <li><code>_self</code>: the current browsing context. (Default)</li>
* <li><code>_blank</code>: usually a new tab, but users can configure
* browsers to open a new window instead.</li>
* <li><code>_parent</code>: the parent browsing context of the current one.
* If no parent, behaves as <code>_self</code>.</li>
* <li><code>_top</code>: the topmost browsing context (the "highest"
* context that’s an ancestor of the current one). If no ancestors, behaves
* as <code>_self</code>.</li>
* </ul>
* </p>
*
* @param target
* the target of this item
*/
public void setTarget(String target) {
if (target == null) {
getElement().removeProperty("target");
} else {
getElement().setProperty("target", target);
}
}

/**
* Sets whether the target URL should be opened in a new browser tab.
* <p>
* This is a convenience method for setting the target to
* <code>_blank</code>. See {@link #setTarget(String)} for more information.
* </p>
*
* @param openInNewBrowserTab
* true if the target URL should be opened in a new browser tab,
* false otherwise
*/
public void setOpenInNewBrowserTab(boolean openInNewBrowserTab) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Browser is maybe a bit redundant? setOpenInNewTab would be enough IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The name has been taken from the AC ticket: vaadin/platform#4850
(ping @rolfsmeds)

Copy link
Contributor

Choose a reason for hiding this comment

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

Tab could be confused with <vaadin-tab>.
Even "browser tab" is a bit misleading, as it could just as well be a window (depending on your browser settings). Not sure if openInNewWindow would be any less misleading?

Copy link
Contributor

Choose a reason for hiding this comment

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

setOpenInNewBrowsingContext would be the most technically correct terminology, I guess, but very unintuitive to most developers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Window seems more misleading to me since opening in a new tab is the default behavior in most browsers.

I agree that Context is unintuitive.

setTarget(openInNewBrowserTab ? "_blank" : null);
}

/**
* Gets whether the target URL should be opened in a new browser tab.
*
* @return true if the target URL should be opened in a new browser tab,
* false otherwise
*/
public boolean isOpenInNewBrowserTab() {
return "_blank".equals(getTarget());
}

private Set<String> getPathAliasesFromView(Class<? extends Component> view,
RouteParameters routeParameters) {
RouteAlias[] routeAliases = view.getAnnotationsByType(RouteAlias.class);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -604,6 +604,39 @@ public void setPathAsComponent_aliasWithMissingParameterNotAdded() {
}, TestRouteWithAliases.class);
}

@Test
public void setTarget_hasTarget() {
sideNavItem.setTarget("_blank");
Assert.assertEquals("_blank",
sideNavItem.getElement().getProperty("target"));
Assert.assertEquals("_blank", sideNavItem.getTarget());
}

@Test
public void targetDefined_setToNull_noTarget() {
sideNavItem.setTarget("_blank");
sideNavItem.setTarget(null);
Assert.assertFalse(sideNavItem.getElement().hasProperty("target"));
Assert.assertNull(sideNavItem.getTarget());
}

@Test
public void setOpenInNewBrowserTab_targetBlankDefinedOnProperty() {
// call setOpenInNewTab and check that getTarget returns "_blank"
sideNavItem.setOpenInNewBrowserTab(true);
Assert.assertEquals("_blank",
sideNavItem.getElement().getProperty("target"));
Assert.assertTrue(sideNavItem.isOpenInNewBrowserTab());
}

@Test
public void openInNewBrowserTabDefined_setOpenInNewBrowserTabToFalse() {
sideNavItem.setOpenInNewBrowserTab(true);
sideNavItem.setOpenInNewBrowserTab(false);
Assert.assertFalse(sideNavItem.getElement().hasProperty("target"));
Assert.assertFalse(sideNavItem.isOpenInNewBrowserTab());
}

private boolean sideNavItemHasLabelElement() {
return sideNavItem.getElement().getChildren()
.anyMatch(this::isLabelElement);
Expand Down