-
Notifications
You must be signed in to change notification settings - Fork 273
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(ui5-link): provide accessible-name support #4711
Conversation
packages/main/src/Link.js
Outdated
* @type {string} | ||
* @defaultvalue "" | ||
* @public | ||
* @since 1.0.0-rc.16 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be 1.2.0 I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
packages/main/src/Link.hbs
Outdated
@@ -13,7 +13,7 @@ | |||
@focusout={{_onfocusout}} | |||
@click={{_onclick}} | |||
@keydown={{_onkeydown}} | |||
@keyup={{_onkeyup}}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not intentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes
@@ -81,4 +81,10 @@ describe("General API", () => { | |||
assert.strictEqual(await link.shadow$("a").getAttribute("aria-haspopup"), "Dialog", "Proper aria-haspopup attribute is set"); | |||
}); | |||
|
|||
it("setting accessible-name on the host is reflected on the link tag", async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"applied on the host element is reflected on the anchor tag" sounds better.
* @since 1.0.0-rc.16 | ||
*/ | ||
accessibleName: { | ||
type: String, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is default value in the documentation, but not in here.
@@ -81,4 +81,10 @@ describe("General API", () => { | |||
assert.strictEqual(await link.shadow$("a").getAttribute("aria-haspopup"), "Dialog", "Proper aria-haspopup attribute is set"); | |||
}); | |||
|
|||
it("setting accessible-name applied on the host element is reflected on the anchor tag", async () => { | |||
const link = await browser.$("#linkAccName").shadow$("а"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try the following:
const link = await browser.$("#linkAccName");
assert.strictEqual(await link.shadow$("a").getAttribute("aria-label"), "Application", "Attribute is reflected");
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no more comments. It looks good from my point of view.
just one more remark, the starting verb f the commit msg should be with small letter - "provide". I will rename that. |
And, as Filip and me commented |
Fixed via the Github UI |
add new property "accessibleName"
Fixes: #4498