Update ways of configuring menu attributes #650

romaricpascal · 2024-01-26T16:39:14Z

Prevent the attributes set through menuAttributes from overriding any of the attributes set internally:

id and role are dropped to limit risks to accessibility
className and class are appended to the class attribute of the menu so it keeps the classes computed by the component

In addition to the update to menuAttributes, the PR also adds a menuClasses. Even if it doubles with with menuAttributes.(class|className), it provides a API with a name consistent with inputClasses and hintClasses from #649 .

romaricpascal · 2024-01-26T16:42:09Z

examples/index.html

@@ -57,6 +57,10 @@
      .custom-hint-class {
        box-shadow: inset 0 0 0 5px #44952e;
      }
+
+      .custom-menu-class {
+        box-shadow: 0 3px 0 0 #fd0, 3px 3px 0 0 #fd0, -3px 3px 0 0 #fd0;


Bit wordy, but extends the yellow focus border to wrap around the menu, not just the input, for a nice visual change.

src/autocomplete.js

colinrotherham · 2024-01-26T18:20:34Z

src/autocomplete.js

+    const menuIsVisible = menuOpen || showNoOptionsFound
+    const menuModifierVisibility = `${menuClassName}--${(menuIsVisible) ? 'visible' : 'hidden'}`
+
+    const safeMenuAttributes = sanitiseMenuAttributes(menuAttributes || {})


Instead of lots of new sanitisation code, can we overwrite role/id instead?

menuAttributes.id = id menuAttributes.role = role

That's a good idea for avoiding to delete the id and the role, didn't think of that. We could do the same with class and className (we'd need to set both, but I don't think that's a massive issue. Could try setting one to null depending on how things work with React/Preact).

I'd still be keen to clone the object to avoid modifying the prop we're given, or do you think it's over the top?

With object spread?

We had {...menuAttributes} on line 538 previously:

https://github.com/alphagov/accessible-autocomplete/blob/40350ae89328e813cc9437e4aac8d52ef813dd4a/src/autocomplete.js#L538C11-L538C30

Are you thinking to overwrite role/id with {...menuAttributes, id, role} instead?

Yeah, just with an object spread, so that when we set the id or role, it doesn't leak the modifications to the object that was passed as props directly. Thinking this'll end up with something like:

// Compute the menu classes from the array // including `menuAttributes.class` and `menuAttributes.className` const menuClasses = ... const computedMenuAttributes = { ...menuAttributes, // Override the attributes the component computes id: `${id}__listbox`, role: 'listbox', className: menuClasses.join(' ') } // Figure a way to delete the `class` property delete computedMenuAttributes.class // Then in the JSX, leaving the handler here for consistency with the other elements // whose handlers are set in JSX. But could equaly go in `computedMenuAttributes` as well. <ul onMouseLeave={(event) => this.handleListMouseLeave(event)} {...computedMenuAttributes} >

Updated with something of the shape I described. Moved onMouseLeave in the computedAttributes to make the JSX on one line in the end.

src/autocomplete.js

in anticipation of pushing optional values to the array before it's joined in the JSX

romaricpascal · 2024-01-29T12:42:27Z

src/autocomplete.js

+      id: `${id}__listbox`,
+      role: 'listbox',
+      className: menuClassList.join(' '),
+      onMouseLeave: this.handleListMouseLeave


Removed the wrapping arrow function, as the handler is bound in the constructor already, so that'll save re-creating listeners each time the menu is re-rendered.

Shall we do the same to handleInputClick?

README.md

colinrotherham · 2024-02-01T10:44:58Z

README.md

+> This option will not override the attributes computed by the component
+> - `id` and `role` are dropped to limit risks to accessibility
+> - `className` and `class` are appended to the `class` attribute of the menu so it keeps the classes computed by the component


I've had a go at re-wording this to include all three id, role and onMouseLeave

Good to explain and link to menuClasses too? Don't worry about explaining the class/className difference since we haven't already done so in the dropdownArrow docs

Suggested change

> This option will not override the attributes computed by the component

> - `id` and `role` are dropped to limit risks to accessibility

> - `className` and `class` are appended to the `class` attribute of the menu so it keeps the classes computed by the component

> To maintain assistive technology support, menu attributes `id`, `role` and `onMouseLeave` cannot be overridden using `menuAttributes`. Setting `className` will append to the component default and [`menuClasses`](#menuClasses) values.

colinrotherham · 2024-02-01T10:47:07Z

src/autocomplete.js

+    if (menuAttributes?.className) {
+      menuClassList.push(menuAttributes.className)
+    }
+
+    if (menuAttributes?.class) {
+      menuClassList.push(menuAttributes.class)
+    }


Looks like class takes precedence in Preact, rather than allowing both?

Suggested change

if (menuAttributes?.className) {

menuClassList.push(menuAttributes.className)

}

if (menuAttributes?.class) {

menuClassList.push(menuAttributes.class)

}

if (menuAttributes?.class || menuAttributes?.className) {

menuClassList.push(menuAttributes?.class || menuAttributes?.className)

}

Yeah, that's a good shout. I'll amend 😊

Added extra test cases while I was at it.

…y the component

romaricpascal · 2024-02-01T17:59:44Z

@colinrotherham Things should be all tidy. The first force-push has the interesting changes, the second being just the tidy up of a stray comma that standard wasn't happy with 😬

colinrotherham · 2024-02-05T12:38:06Z

README.md

+
+Type: `string | null`
+
+Adds custom html classes to the menu elements.


We just say "menu" elsewhere but should we fix ~~elements~~ → element, or be specific like menuAttributes?

Suggested change

Adds custom html classes to the menu elements.

Adds custom html classes to the generated `ul` menu element.

Updated both here and for menuAttributes 😊

Even better, thank you

romaricpascal force-pushed the menu-attributes-update branch 4 times, most recently from de4763e to 7cfc175 Compare January 26, 2024 16:56

romaricpascal commented Jan 26, 2024

View reviewed changes

romaricpascal marked this pull request as ready for review January 26, 2024 16:57

romaricpascal added this to the Next milestone Jan 26, 2024

colinrotherham reviewed Jan 26, 2024

View reviewed changes

src/autocomplete.js Outdated Show resolved Hide resolved

romaricpascal force-pushed the styling-props branch 2 times, most recently from b972a2d to d1967d5 Compare January 29, 2024 09:55

Refactor how the menu class is generated

4e3fad7

in anticipation of pushing optional values to the array before it's joined in the JSX

romaricpascal force-pushed the menu-attributes-update branch from 7cfc175 to 9eb5e1d Compare January 29, 2024 12:29

romaricpascal commented Jan 29, 2024

View reviewed changes

colinrotherham reviewed Jan 29, 2024

View reviewed changes

README.md Outdated Show resolved Hide resolved

Base automatically changed from styling-props to main January 30, 2024 16:52

Add menuClasses option and corresponding test

c3a705d

romaricpascal force-pushed the menu-attributes-update branch from 9eb5e1d to 7c105ae Compare January 30, 2024 17:04

romaricpascal requested a review from colinrotherham February 1, 2024 10:12

colinrotherham reviewed Feb 1, 2024

View reviewed changes

romaricpascal force-pushed the menu-attributes-update branch from 7c105ae to d8f8b77 Compare February 1, 2024 17:53

Prevent menuAttributes from overriding attributes set automatically b…

ce2a61c

…y the component

romaricpascal force-pushed the menu-attributes-update branch from 4db7b55 to d031e68 Compare February 1, 2024 17:57

romaricpascal requested a review from colinrotherham February 5, 2024 10:43

colinrotherham reviewed Feb 5, 2024

View reviewed changes

romaricpascal force-pushed the menu-attributes-update branch from d031e68 to 7ed8b6b Compare February 6, 2024 13:29

Remove unnecessary arrow function just forwarding to a bound function

25cc4a3

romaricpascal force-pushed the menu-attributes-update branch from 7ed8b6b to 25cc4a3 Compare February 6, 2024 13:32

colinrotherham approved these changes Feb 6, 2024

View reviewed changes

romaricpascal merged commit 9079e77 into main Feb 12, 2024
3 checks passed

romaricpascal deleted the menu-attributes-update branch February 12, 2024 10:46

romaricpascal mentioned this pull request Apr 19, 2024

v3.0.0 #693

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update ways of configuring menu attributes #650

Update ways of configuring menu attributes #650

romaricpascal commented Jan 26, 2024 •

edited

romaricpascal Jan 26, 2024

colinrotherham Jan 26, 2024

romaricpascal Jan 29, 2024

colinrotherham Jan 29, 2024

romaricpascal Jan 29, 2024

romaricpascal Jan 29, 2024

romaricpascal Jan 29, 2024

colinrotherham Feb 1, 2024

colinrotherham Feb 1, 2024

colinrotherham Feb 1, 2024

romaricpascal Feb 1, 2024

romaricpascal Feb 1, 2024

romaricpascal commented Feb 1, 2024

colinrotherham Feb 5, 2024

romaricpascal Feb 6, 2024

colinrotherham Feb 6, 2024


		Type: `string \| null`

		Adds custom html classes to the menu elements.

	Adds custom html classes to the menu elements.
	Adds custom html classes to the generated `ul` menu element.

Update ways of configuring menu attributes #650

Update ways of configuring menu attributes #650

Conversation

romaricpascal commented Jan 26, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romaricpascal commented Feb 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romaricpascal commented Jan 26, 2024 •

edited