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

Conversation

DiegoCardoso
Copy link
Contributor

@DiegoCardoso DiegoCardoso commented Jan 16, 2024

Description

Add two new API for setting/getting the target property of a SideNavItem, whose value is used to set the target attribute in the internal anchor element inside the web-component:

  • setTarget(String)/getTarget() - which receives any string as argument
  • setOpenInNewBrowserTab(boolean)/isOpenInNewBrowserTab()- a convenience method that internally calls setTarget with _blank as value when called with true.

Depends on vaadin/web-components#7088
Fixes #5091
AC vaadin/platform#4850

Type of change

  • Bugfix
  • Feature

@DiegoCardoso DiegoCardoso force-pushed the feat/side-nav/target-api branch 2 times, most recently from 4fb2992 to ff45bb3 Compare January 16, 2024 13:55
* 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.

@DiegoCardoso DiegoCardoso enabled auto-merge (squash) January 22, 2024 13:46
Copy link

sonarcloud bot commented Jan 22, 2024

Quality Gate Passed Quality Gate passed

The SonarCloud Quality Gate passed, but some issues were introduced.

1 New issue
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@DiegoCardoso DiegoCardoso merged commit 1951cf1 into main Jan 22, 2024
5 checks passed
@DiegoCardoso DiegoCardoso deleted the feat/side-nav/target-api branch January 22, 2024 13:55
@vaadin-bot
Copy link
Collaborator

This ticket/PR has been released with Vaadin 24.4.0.alpha3 and is also targeting the upcoming stable 24.4.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.

SideNavItem should have API for setting target
4 participants