Feature/cdd 2285 - Addition of About Tab and dropdown functionality on mobile #588
Conversation
- configured the new dropdown tab and started to make use of the GDS functionality for dropdown boxes. Next steps are to test and finalise the CSS functionality
Unit tests coverage
|
luketowell
force-pushed
the
feature/CDD-2285
branch
4 times, most recently
from
fbb71d4 to
0fef2bb
Compare
luketowell
force-pushed
the
feature/CDD-2285
branch
from
0fef2bb to
baa7262
Compare
luketowell
force-pushed
the
feature/CDD-2285
branch
2 times, most recently
from
db773d0 to
27db6f1
Compare
… functionality, new test required for mobile dropdown
luketowell
force-pushed
the
feature/CDD-2285
branch
from
27db6f1 to
22001e1
Compare
…onality and the download functionality
| onChange={onChangeFunction} | ||
| className={className} | ||
| data-testid="DropdownSelect" | ||
| aria-label="Label for the select element" |
There was a problem hiding this comment.
What is this aria-label for ?
There was a problem hiding this comment.
The aria label is there because the input doesn't require a label however aria accessibility standards specify that all "input" elements require either a label or an aria-label
There was a problem hiding this comment.
I suppose other than the aria-label I could have a element but set it to hidden. I am not sure which is the better option
There was a problem hiding this comment.
aria-label is the accessible name that provides context to users using assistive technologies (like screen readers). If you're on a Mac I'd suggest enabling VoiceOver (Cmd+f5) and tabbing to this element to see how this might be confusing for a blind/visually impaired user. We need to be descriptive, but without overloading with information (similar to good UI/UX design for non-visually impaired users)
I'd suggest something like: “Select data view", “Choose display option” or "Select content section "
Could expand this further by appending the chart card title to it e.g.
- “Select data view for [Chart Name]”
- “Choose display option for [Chart Name]”
- “Select content section for [Chart Name]”
[Chart Name] being dynamic (i.e. Choose display option for Daily deaths with COVID-19 on the death certificate by date of death.)
There was a problem hiding this comment.
This has been renamed to Choose display option for [Chart Name] which I agree is more readable.
| const displayCorrespondingContent = async (selectedValue: string) => { | ||
| // create a new array with from the original dropDown options. this prevents the splice from altering the original array data. | ||
| const options = dropdownOptions.slice() | ||
| // find the index of the selectedValue and remove the selectedValue object from the array. | ||
| const optionIndex = options.findIndex((option) => option.value === selectedValue) | ||
| options.splice(optionIndex, 1) | ||
|
|
||
| options.map((option) => { | ||
| const nonSelectedContent = document.getElementById(`${option.value}-${chartIdentifier}-content`) | ||
| if (nonSelectedContent) { | ||
| nonSelectedContent.setAttribute('data-state', 'inactive') | ||
| } | ||
| }) | ||
|
|
||
| const selectedContent = document.getElementById(`${selectedValue}-${chartIdentifier}-content`) | ||
| if (selectedContent) { | ||
| selectedContent.setAttribute('data-state', 'active') | ||
| } | ||
| } | ||
|
|
||
| const onChangeFunction = async (optionSelected: ChangeEvent<HTMLSelectElement>) => { | ||
| optionSelected.preventDefault() | ||
| await displayCorrespondingContent(optionSelected.target.value) | ||
| } |
There was a problem hiding this comment.
Just wondering why we're using DOM query selectors? React is designed specifically so we don't have to do this and allows us manipulate the DOM in a safe and immutable fashion.
There was a problem hiding this comment.
I used the setAttribute because I couldn't figure out another way to switch between content without manually setting the attribute whilst also enabling the tab functionality in the library that we use on the full screen display. I am happy to make the change if you can suggest a better way of doing it.
There was a problem hiding this comment.
Hey @8lane, I have refractored this completely and changed how state is handled in order to change the tab from the dropdown. Please let me know if you think it needs other tweaks.
luketowell
changed the title
Some responsive issues |
| { value: 'about', displayText: 'About' }, | ||
| ] | ||
|
|
||
| const onChangeFunction = async (optionSelected: ChangeEvent<HTMLSelectElement>) => { |
There was a problem hiding this comment.
In React, these callback functions would typically be named with "handle" + the action. e.g. handleChange
However, given its simplicity, I would just put this function inline inside the onChange
- You don't need to type the event then
- Easier to read as it's in order of execution
There was a problem hiding this comment.
Moved in line and tested 😀
| className={className} | ||
| aria-label={`${chartIdentifier} select element`} | ||
| > | ||
| {dropdownOptions.map(({ value, displayText }: DropdownOptionsProps, index) => ( |
There was a problem hiding this comment.
DropdownOptionsProps is redundant here. Typescript will infer this automatically
|
|
||
| return ( | ||
| <select | ||
| id={`#dropdown-${chartIdentifier}`} |
There was a problem hiding this comment.
Maybe make this a bit more unique, just incase. Usually we prefix ids e.g. ukhsa-chart-dropdown-{id}
There was a problem hiding this comment.
Id has been updated following your suggestion 👍
…downTabOptions type declaration
|
Description
This change moves the chart description from the header into a newly created About tab. It also includes manipulating the tabs depending on screen resolution to ensure that drop-downs are displayed on smaller devices so that mobile users can easily navigate between content.
Fixes #CDD-2285
Type of change
How Has This Been Tested?
Checklist: