amp-sidebar: optional attribute to change text for screen-reader clos… #9958
Conversation
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed, please reply here (e.g.
|
package.json
Outdated
| @@ -51,7 +51,7 @@ | |||
| "formidable": "1.0.17", | |||
| "fs-extra": "0.27.0", | |||
| "glob": "7.1.2", | |||
| "gulp": "3.9.1", | |||
| "gulp": "^3.9.1", | |||
There was a problem hiding this comment.
Seems we got some extra changes in this commit. Please revert this file.
src/service/platform-impl.js
Outdated
| @@ -98,6 +98,14 @@ export class Platform { | |||
| } | |||
|
|
|||
| /** | |||
| * Whether the current browser is based on the WebKit engine. | |||
There was a problem hiding this comment.
Seems we got some extra changes in this commit. Please revert this file.
yarn.lock
Outdated
| @@ -48,10 +48,6 @@ acorn-jsx@^3.0.0: | |||
| dependencies: | |||
There was a problem hiding this comment.
Seems we got some extra changes in this commit. Please revert this file.
| @@ -122,10 +122,19 @@ export class AmpSidebar extends AMP.BaseElement { | |||
| } | |||
| }); | |||
|
|
|||
| var ariaLabel = this.element.getAttribute('data-close-button-aria-label') | |||
There was a problem hiding this comment.
This should be marked as const:
const ariaLabel = ...| @@ -121,10 +121,17 @@ export class AmpSidebar extends AMP.BaseElement { | |||
| } | |||
| }); | |||
|
|
|||
| var ariaLabel = this.element.getAttribute('data-close-button-aria-label') | |||
There was a problem hiding this comment.
This should be marked as const:
const ariaLabel = ...| expect(impl.close_).to.have.not.been.called; | ||
| closeButton.click(); | ||
| expect(impl.close_).to.be.calledOnce; | ||
|
|
||
| if (options.closeText) { ampsetbar.setAttribute('data-close-button-aria-label', options.closeText) }; |
There was a problem hiding this comment.
Please put the if statement body on a new line:
if (option.closeText) {
ampsetbar.setAttribute(...);
}| // Invisible close button at the end of sidebar for screen-readers. | ||
| const screenReaderCloseButton = this.document_.createElement('button'); | ||
| // TODO(aghassemi, #4146) i18n | ||
| screenReaderCloseButton.textContent = 'Close the sidebar'; | ||
| if(ariaLabel) { |
| // TODO(aghassemi, #4146) i18n | ||
| screenReaderCloseButton.textContent = 'Close the sidebar'; | ||
| if(ariaLabel) { |
|
CLAs look good, thanks! |
There was a problem hiding this comment.
Hi, I got an email telling me to make the following changes, so I did, I'll cut and paste it here.
@jridgewell requested changes on this pull request.
In package.json:
@@ -51,7 +51,7 @@
"formidable": "1.0.17",
"fs-extra": "0.27.0",
"glob": "7.1.2",
- "gulp": "3.9.1",
- "gulp": "^3.9.1",
Seems we got some extra changes in this commit. Please revert this file.
In src/service/platform-impl.js:
@@ -98,6 +98,14 @@ export class Platform {
}
/**
-
- Whether the current browser is based on the WebKit engine.
Seems we got some extra changes in this commit. Please revert this file.
- Whether the current browser is based on the WebKit engine.
In yarn.lock:
@@ -48,10 +48,6 @@ acorn-jsx@^3.0.0:
dependencies:
Seems we got some extra changes in this commit. Please revert this file.
In extensions/amp-sidebar/1.0/amp-sidebar.js:
@@ -122,10 +122,19 @@ export class AmpSidebar extends AMP.BaseElement {
}
});
- var ariaLabel = this.element.getAttribute('data-close-button-aria-label')
This should be marked as const:
const ariaLabel = ...
In extensions/amp-sidebar/0.1/amp-sidebar.js:
@@ -121,10 +121,17 @@ export class AmpSidebar extends AMP.BaseElement {
}
});
- var ariaLabel = this.element.getAttribute('data-close-button-aria-label')
This should be marked as const:
const ariaLabel = ...
In extensions/amp-sidebar/0.1/test/test-amp-sidebar.js:
expect(impl.close_).to.have.not.been.called;
closeButton.click();
expect(impl.close_).to.be.calledOnce;
-
if (options.closeText) { ampsetbar.setAttribute('data-close-button-aria-label', options.closeText) };
Please put the if statement body on a new line:
if (option.closeText) {
ampsetbar.setAttribute(...);
}
In extensions/amp-sidebar/0.1/amp-sidebar.js:
// Invisible close button at the end of sidebar for screen-readers.
const screenReaderCloseButton = this.document_.createElement('button');
// TODO(aghassemi, #4146) i18n
- screenReaderCloseButton.textContent = 'Close the sidebar';
- if(ariaLabel) {
Space after if
In extensions/amp-sidebar/1.0/amp-sidebar.js:
// TODO(aghassemi, #4146) i18n
- screenReaderCloseButton.textContent = 'Close the sidebar';
- if(ariaLabel) {
Space after if
| if (options.closeText) { | ||
| ampsetbar.setAttribute('data-close-button-aria-label', options.closeText) | ||
| }; | ||
| expect(closeButton.textContent).to.not.equal(!('Close the sidebar')); |
There was a problem hiding this comment.
This test will have to be done in a separate it test.
There was a problem hiding this comment.
Would this it test be okay?
it('should replace text to screen reader \
button in data-close-button-aria-label', () => {
return getAmpSidebar().then(obj =>) {
if (options.closeText) {
ampsetbar.setAttribute('data-close-button-aria-label', options.closeText)
}
});
});
There was a problem hiding this comment.
The options.closeText part will have to be moved into getAmpSidebar, but that should be most of it.
There was a problem hiding this comment.
Do you mean pass it to getAmpSidebar?
| it('should replace text to screen reader \ | ||
| button in data-close-button-aria-label', () => { | ||
| return getAmpSidebar().then(obj => { | ||
| if (options.closeText) { |
There was a problem hiding this comment.
This will need to be moved inside the getAmpSidebar function.
| if (options.closeText) { | ||
| ampsetbar.setAttribute('data-close-button-aria-label', options.closeText); | ||
| }; | ||
| expect(closeButton.textContent).to.not.equal(!('Close the sidebar')); |
There was a problem hiding this comment.
expect(closeButton.textContent).to.equal(options.closeText);
| it('should replace text to screen reader \ | ||
| button in data-close-button-aria-label', () => { | ||
| return getAmpSidebar().then(obj => { | ||
| if (options.closeText) { |
There was a problem hiding this comment.
These next 3 lines sill need to be moved inside getAmpSidebar.
| // Invisible close button at the end of sidebar for screen-readers. | ||
| const screenReaderCloseButton = this.document_.createElement('button'); | ||
| // TODO(aghassemi, #4146) i18n | ||
| screenReaderCloseButton.textContent = 'Close the sidebar'; | ||
| if (ariaLabel) { |
There was a problem hiding this comment.
please remove the //TODO line above since it is fixed by this PR
| @@ -121,10 +121,17 @@ export class AmpSidebar extends AMP.BaseElement { | |||
| } | |||
| }); | |||
|
|
|||
| const ariaLabel = this.element.getAttribute('data-close-button-aria-label') | |||
There was a problem hiding this comment.
I'd recommend:
const ariaLabel = this.element.getAttribute('data-close-button-aria-label') || 'Close the sidebar';
for consistency with other places that do this and also now you can remove the if/else below.
| @@ -126,6 +126,10 @@ This attribute is present when the sidebar is open. | |||
|
|
|||
| This element includes [common attributes](https://www.ampproject.org/docs/reference/common_attributes) extended to AMP components. | |||
|
|
|||
| ##### data-close-button-aria-label attribute | |||
There was a problem hiding this comment.
##### data-close-button-aria-label**
Optional string used as ARIA label for close button.
for consistency with other places that have this attribute (e.g. amp-image-lightbox)
also please move this above the ##### common attributes
|
@rhapsodyai Please also revert the changes to |
|
I am getting
when I use:
How should I replace this usage? |
|
@jridgewell @aghassemi Please let me know if the changes I made are ok. |
| @@ -121,10 +121,14 @@ export class AmpSidebar extends AMP.BaseElement { | |||
| } | |||
| }); | |||
|
|
|||
| // replacement label for invisible close button set value in amp sidebar | |||
There was a problem hiding this comment.
nit: capitalize first letter for consistency with other comments
| return getAmpSidebar().then(obj => { | ||
| const sidebarElement = obj.ampSidebar; | ||
| const closeButton = sidebarElement.lastElementChild; | ||
| if (options.closeText) { |
There was a problem hiding this comment.
this part needs to move to the getAmpSidebar() method. Then this test becomes:
getAmpSidebar({closeText: 'custom close label'}). then(...
...
expect(closeButton.textContent).to.equal( 'custom close label');
| @@ -122,10 +122,14 @@ export class AmpSidebar extends AMP.BaseElement { | |||
| } | |||
| }); | |||
|
|
|||
| // replacement label for invisible close button set value in amp sidebar | |||
There was a problem hiding this comment.
ditto same comment above
| return getAmpSidebar().then(obj => { | ||
| const sidebarElement = obj.ampSidebar; | ||
| const closeButton = sidebarElement.lastElementChild; | ||
| if (options.closeText) { |
| @@ -122,6 +122,10 @@ Specifies the display layout of the sidebar, which must be `nodisplay`. | |||
|
|
|||
| This attribute is present when the sidebar is open. | |||
|
|
|||
| ##### data-close-button-aria-label** | |||
|
|
|||
| This element includes the value of the aria label attribute for the screenReaderCloseButton text. | |||
There was a problem hiding this comment.
for consistency with other places we document this attribute please change to:
Optional string used as ARIA label for close button.
|
@rhapsodyai This looks pretty good just a few more requests and then we can merge. Thanks! |
|
@aghassemi Everything should have been updated. Please let me know if there is something else. |
| @@ -122,6 +122,10 @@ Specifies the display layout of the sidebar, which must be `nodisplay`. | |||
|
|
|||
| This attribute is present when the sidebar is open. | |||
|
|
|||
| ##### data-close-button-aria-label** | |||
|
|
|||
| Optional string used as ARIA label for close button. | |||
There was a problem hiding this comment.
This should be more like
"Optional attribute used to set ARIA label for the close button added for accessibility"
CC @bpaduch - to review the string
|
@rhapsodyai - your PR has lint errors , you will need to fix this to make this pull request become merge-able Overall the changes look good! Thanks for contributing |
|
@camelburrito Thank you, it should be mergeable now. |
| 'data-close-button-aria-label'}).then(obj => { | ||
| const sidebarElement = obj.ampSidebar; | ||
| const closeButton = sidebarElement.lastElementChild; | ||
| if (options.closeText) { |
There was a problem hiding this comment.
@rhapsodyai the following lines:
if (options.closeText) {
obj.ampSidebar.setAttribute('data-close-button-aria-label',
options.closeText);
};
should be moved to the getAmpSidebar function definition (line 44 of this file). you can put it right after the if (options.open) { ampSidebar.setAttribute('open', ''); } line.
A bit of background: getAmpSidebar actually creates the sidebar component and since tests may want different configurations of the sidebar component, it already takes an option argument.
|
@rhapsodyai everything looks great, merging now. Thanks! |
aghassemi
dismissed stale reviews from camelburrito and jridgewell
comments addressed
…e button. #9867
Fixed amp-sidebar: optional attribute to change text for screen-reader close button. #9867
Fixes #9867.