feat: add target property to side-nav-item #7088
Conversation
DiegoCardoso
force-pushed
the
feat/side-nav/target-attr
branch
from
46a5aac
to
3763ae8
Compare
| /** | ||
| * The target of the link. Works only when `path` is set. | ||
| */ | ||
| target: string | null | undefined; |
There was a problem hiding this comment.
Should there be a type here that only accepts something like '_self' | '_blank' | '_parent' | '_top'?
There was a problem hiding this comment.
target in HTMLAnchoElement doesn't seem to get any special type (source)
There was a problem hiding this comment.
A niche use-case would be to configure a specific browsing context, which could have any name apparently (vaadin/flow-components#5091 (comment)). So the union type would need to include string as well. But just starting with string is fine IMO 👍
| it('should not set target attribute to the anchor when target is empty string', async () => { | ||
| item.target = ''; | ||
| await nextRender(); | ||
| expect(anchor.getAttribute('target')).to.be.not.ok; |
There was a problem hiding this comment.
From my testing the attribute is still defined for an empty string, the assertion here doesn't work as intended. Would be better to use hasAttribute for checking the attribute existence IMO.
If we want to avoid defining the attribute it requires further changes. Don't see why it would be necessary though. So maybe just remove the test case.
There was a problem hiding this comment.
Yeah, you're right. Based on this comment, I also changed the assertion in the previous test to use hasAttribute instead.
|
|
This ticket/PR has been released with Vaadin 24.4.0. |
Description
Make it possible to define a
targetattribute to forward its value to the internal anchor element present in the<vaadin-side-nav-item>component.Part of vaadin/flow-components#5091
Type of change