Fixes #6679 - patch to make component accessible

[shristit]

No description provided.

[willdurand]

This looks like a good start, thanks!

Something needs to be fixed on Travis CI, you should take a look: it must be green :)
In addition, you need to write new test cases.

[championshuttler]

Hi @shristit thanks for your PR , and welcome to AMO community 🎉 . Before any maintainer review the PR, I am suggesting few changes to help you. It almost looks good. I added few comments , can you please update the PR according to those. Also you can run yarn prettier-dev before pushing the changes

[championshuttler]

Can you please remove the extra space and add a comma after title please :)

[championshuttler]

Nit: Can you please change it to button .

[championshuttler]

You have some trailing space here , can you please remove it.

[willdurand]

@shristit hi, do you need help to update your PR?

[shristit]

Hi I will be back to it next week, Have college exams

…

On Tue, Oct 23, 2018 at 4:37 PM William Durand ***@***.***> wrote: @shristit <https://github.com/shristit> hi, do you need help to update your PR? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#6693 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AoV8GMZYlMPc4P8ODEHYPXDxhzAeXHbYks5unviLgaJpZM4XxIBw> .

[codecov-io]

Codecov Report

Merging #6693 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #6693      +/-   ##
==========================================
+ Coverage   97.84%   97.84%   +<.01%     
==========================================
  Files         249      241       -8     
  Lines        6810     6556     -254     
  Branches     1266     1243      -23     
==========================================
- Hits         6663     6415     -248     
+ Misses        133      126       -7     
- Partials       14       15       +1

Impacted Files	Coverage Δ
src/ui/components/Rating/index.js	100% <100%> (ø)	⬆️
src/amo/components/HomeHeroBanner/index.js	88.88% <0%> (-11.12%)	⬇️
src/core/languages.js	75% <0%> (-10.72%)	⬇️
src/core/client/base.js	63.63% <0%> (-8.59%)	⬇️
src/amo/components/App/index.js	91.8% <0%> (-6.54%)	⬇️
src/amo/components/RatingManager/index.js	96.55% <0%> (-3.45%)	⬇️
src/core/logger.js	80% <0%> (-2.36%)	⬇️
src/core/server/base.js	61.45% <0%> (-1.26%)	⬇️
src/core/reducers/addons.js	98.71% <0%> (-0.22%)	⬇️
src/core/installAddon.js	99.15% <0%> (-0.09%)	⬇️
... and 138 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7a1c378...131f1fe. Read the comment docs.

[willdurand]

@shristit that looks good, but you need to write some tests now :)

[shristit]

Hi William , I do not know how to write test case can you help me with that please? Thanks

[willdurand]

Hi William , I do not know how to write test case can you help me with that please?

@shristit Yep. So you need to edit this file: https://github.com/mozilla/addons-frontend/blob/c543050abd5b430018c100a1ee7640a32b5231c8/tests/unit/ui/components/TestRating.js

You can see a list of functions starting with it(). Each function is a test case. We need to write one test case per "thing" we add/change/fix. In your case, you changed the button to not only be a button but to also have an associated span.

I think it would make sense to update this test case:

addons-frontend/tests/unit/ui/components/TestRating.js

Lines 186 to 193 in c543050

	it('renders all stars as selectable by default', () => {
	const root = renderWithRating();
	[1, 2, 3, 4, 5].forEach((rating) => {
	const star = getStar({ root, rating });
	expect(star).toHaveClassName('Rating-star');
	expect(star.type()).toEqual('button');
	});
	});

You can make sure the button has a prop called aria-describedby, with Rating-rating-${rating}-title as value (hint: you can use toHaveProp()).

You should also look for a span withe the same id.

[willdurand]

Hi @shristit, do you need help? Here is a test case you could write:

diff --git a/tests/unit/ui/components/TestRating.js b/tests/unit/ui/components/TestRating.js
index 45f891b96..c57268f50 100644
--- a/tests/unit/ui/components/TestRating.js
+++ b/tests/unit/ui/components/TestRating.js
@@ -192,6 +192,26 @@ describe(__filename, () => {
     });
   });
 
+  it('renders an accessible button for each rating', () => {
+    const root = renderWithEmptyRating();
+    [1, 2, 3, 4, 5].forEach((rating) => {
+      const button = getStar({ root, rating });
+      const id = `Rating-rating-${rating}-title`;
+
+      expect(button).toHaveProp('aria-describedby', id);
+
+      // Each rating should have a `span` along with a button.
+      expect(root.find(`span[id="${id}"]`)).toHaveLength(1);
+      expect(root.find(`span[id="${id}"]`)).toHaveProp(
+        'className',
+        'visually-hidden',
+      );
+      expect(root.find(`span[id="${id}"]`)).toHaveText(
+        `Rate this add-on ${rating} out of 5`,
+      );
+    });
+  });
+
   it('renders an accessible description for null stars', () => {
     const root = renderWithEmptyRating();

[willdurand]

Awesome, thanks @shristit! Did you understand the test case?

[caitmuenster]

Yay, thanks for the patch @shristit! 🎉 Your contribution has been added to our recognition wiki.

Would you be interested in creating an account on mozillians.org? I'd be happy to vouch for you.