Fix aria expanded and height of option select #1146
Conversation
andysellick
changed the title
|
Could we create a failing test for this? |
|
On it. |
andysellick
changed the title
| @@ -86,6 +86,10 @@ describe('GOVUK.OptionSelect', function() { | |||
| expect(optionSelect.isClosed()).toBe(false); | |||
| }); | |||
|
|
|||
| it ('has called setupHeight as part of initialisation', function(){ | |||
There was a problem hiding this comment.
Can we make the test name to what we're testing, which is that it's expanded by default, and update the test to reflect this?
andysellick
changed the title
andysellick
force-pushed
the
fix-options-select-aria-and-height
branch
from
cb82833 to
95edbcd
Compare
andysellick
changed the title
|
@andysellick It's helpful to include links for review like: |
| @@ -86,6 +86,17 @@ describe('GOVUK.OptionSelect', function() { | |||
| expect(optionSelect.isClosed()).toBe(false); | |||
| }); | |||
|
|
|||
| it ('sets the height of the options container as part of initialisation', function(){ | |||
There was a problem hiding this comment.
These tests could communicate the bug we're fixing, if we named this test around how we're checking the component is expanded correctly we'd notice that we're testing the height is set but we're not testing that the aria-expanded attribute is set properly.
There was a problem hiding this comment.
We need a separate second test to check that aria-expanded has the correct value(s)
There was a problem hiding this comment.
The first two tests in the file now cover this for both open and closed initial states.
| @@ -124,12 +135,6 @@ describe('GOVUK.OptionSelect', function() { | |||
| expect($optionSelectHTML.hasClass('js-closed')).toBe(false); | |||
| }); | |||
|
|
|||
| it ('calls setupHeight()', function(){ | |||
There was a problem hiding this comment.
This test assumes setupHeight gets called when the option select is opened, but that's now not always the case. setupHeight is called on init if the option select is in the expanded state, but not if it's collapsed. I'm writing other tests to fill this gap.
There was a problem hiding this comment.
Have now put this test back in a modified form.
|
Visual regression picked up on the changes: |
andysellick
force-pushed
the
fix-options-select-aria-and-height
branch
from
95edbcd to
85b9039
Compare
andysellick
force-pushed
the
fix-options-select-aria-and-height
branch
from
85b9039 to
5896df6
Compare
|
@andysellick can you put in the PR which browsers/devices you've tested the CSS changes against? |
andysellick
force-pushed
the
fix-options-select-aria-and-height
branch
from
5896df6 to
27cb21c
Compare
- set aria-expanded to true by default when initialising rather than performing a check as it will always be open on load, unless it's set to be closed in which case the code will change it back to false anyway as it closes it - fix for height issue by calling setheight on initialisation, code is structured already so that it won't get called again unless it needs to be - fixes #860
- remove test to check setupheight is called on open, as now called on init - add test to check that a height is set during init
- use display block on labels rather than inline block and having to set widths - add a bit of padding right to labels to smarten them up a bit - change overflow-y from scroll to auto on label container so scrollbar isn't always present
andysellick
force-pushed
the
fix-options-select-aria-and-height
branch
from
27cb21c to
a161dae
Compare
Before:
After:
https://govuk-static-pr-1146.herokuapp.com/component-guide/option_select