Conversation
catvine-stormid
left a comment
catvine-stormid
left a comment
There was a problem hiding this comment.
Looks good! 🎉
Only a few tiny nitpicky comments, which may or may not be needed, so overall looks ready to go.
| }, | ||
| bannerTemplate(model){ | ||
| return `<section role="dialog" aria-live="polite" aria-label="Cookies" class="${model.classNames.banner}"> | ||
| return `<section role="region" aria-live="polite" aria-label="Cookies" class="${model.classNames.banner}"> |
There was a problem hiding this comment.
The default banner container has been changed to a <div> element with role="region" attribute elsewhere throughout the code, rather than a <section> (README, defaults.js file). Is it worth updating here too for consistency?
| expect(document.querySelector(`.${defaults.classNames.banner}`).getAttribute('role')).toEqual('region'); | ||
| }); | ||
|
|
||
| it('The banner should have be polite aria live region', async () => { |
There was a problem hiding this comment.
Nitpicking here! But seems to be a small typo, might be clearer to say "The banner should be a polite aria live region"?
There was a problem hiding this comment.
This is in regards to the test starting on line 78, just for clarification.
| getState: Store.getState, | ||
| getState: store.getState, | ||
| showBanner(cb) { | ||
| showBanner(Store)(cb); |
There was a problem hiding this comment.
Culling of the capital letters everywhere! Looks much tidier in all lowercase, I have to say. Good stuff :)
2 participants
Resolves #282.