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(ui5-link): provide accessible-name support #4711

Merged
merged 9 commits into from
Feb 10, 2022
Merged

Conversation

Todor-ads
Copy link
Member

@Todor-ads Todor-ads commented Feb 8, 2022

add new property "accessibleName"

Fixes: #4498

* @type {string}
* @defaultvalue ""
* @public
* @since 1.0.0-rc.16
Copy link
Contributor

@unazko unazko Feb 8, 2022

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@@ -13,7 +13,7 @@
@focusout={{_onfocusout}}
@click={{_onclick}}
@keydown={{_onkeydown}}
@keyup={{_onkeyup}}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Not intentional?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes

packages/main/test/pages/Link.html Outdated Show resolved Hide resolved
@@ -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 () => {
Copy link
Contributor

@unazko unazko Feb 8, 2022

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,
Copy link
Contributor

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.

@Todor-ads Todor-ads requested a review from unazko February 8, 2022 13:46
packages/main/src/Link.js Outdated Show resolved Hide resolved
@@ -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$("а");
Copy link
Member

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");

packages/main/src/Link.js Outdated Show resolved Hide resolved
unazko
unazko previously approved these changes Feb 9, 2022
Copy link
Contributor

@unazko unazko left a 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.

@ilhan007
Copy link
Member

ilhan007 commented Feb 9, 2022

just one more remark, the starting verb f the commit msg should be with small letter - "provide". I will rename that.

packages/main/src/Link.js Outdated Show resolved Hide resolved
@ilhan007
Copy link
Member

ilhan007 commented Feb 9, 2022

And, as Filip and me commented defaultValue: "" is not needed

ilhan007
ilhan007 previously approved these changes Feb 9, 2022
@ilhan007 ilhan007 changed the title feat(ui5-link): Provide accessible-name support feat(ui5-link): provide accessible-name support Feb 9, 2022
@ilhan007
Copy link
Member

ilhan007 commented Feb 9, 2022

And, as Filip and me commented defaultValue: "" is not needed

Fixed via the Github UI

@ilhan007 ilhan007 merged commit 6597afa into master Feb 10, 2022
@ilhan007 ilhan007 deleted the add_accessible_name branch February 10, 2022 08:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ui5-link does not have accessible-name support
5 participants