Give each room directory entry the listitem role to correspond with the containing list.

[ndarilek]

Signed-off-by: Nolan Darilek [email protected]

---

Here's what your changelog entry will look like:

✨ Features

• Give each room directory entry the listitem role to correspond with the containing list. (#7035). Contributed by @ndarilek.

Preview: https://617b0ff6ff9a2313f1d9f939--matrix-react-sdk.netlify.app
⚠️ Do you trust the author of this PR? Maybe this build will steal your keys or give you malware. Exercise caution. Use test accounts.

[t3chguy]

So unfortunately this change has entirely broken the visual layout of the panel

display: contents; on the listitem seems to resolve it though.

[ndarilek]

Ugh, had no idea that just adding a div would do that. Is there an easy fix?

[t3chguy]

@ndarilek yes, display: contents; on the listitem seems to do the trick

[ndarilek]

OK, need me to add it or do you have it?

[t3chguy]

would be great if you could do it so that it doesn't need yet another reviewer to step in

[ndarilek]

OK, think I got it but can't confirm visually. Please let me know if that did it.

[ndarilek]

Oh, no it doesn't, looks like React expects to typecheck styles. Haven't done React in a while. Let me refresh my memory on how to inline this.

[ndarilek]

I'll give it a shot. I've never done CSS mainly because I can't verify the results. Should it be scoped to RoomDirectory, or should it be more global? I could imagine adding more elements for better ARIA support, so some sort of screen reader/accessibility-only rule might be best. If so, where should it go?

[t3chguy]

The className could be something like mx_RoomDirectory_listItem, the relevant stylesheet is called _RoomDirectory.scss

The rule would look something like

.mx_RoomDirectory_listItem {
    display: contents;
}

with a new prop in the React replacing the inline style prop: className="mx_RoomDirectory_listItem" - hope that helps

[ndarilek]

Gotcha. Next question: should that be a global rule, something like: ``` .mx_A11yOnly {     display: contents; } ``` Because I could see adding other divs/spans for accessibility-only purposes, and a need to indicate in more than one place that this particular rule exists for that specific reason. Thanks.

[t3chguy]

display: contents; won't be the right thing for all situations, in most cases it won't be required, and we also avoid general global rules in the first place

[ndarilek]

Understood, will get to it soon. Thanks for the help!

[ndarilek]

Think this should be good to go. Please let me know if I missed anything.

BTW, I didn't notice the failing CI lint because I've been getting emails like this:

Develop: All jobs have failed

Viewing the linked workflow run displays:

'auto-push' is enabled but 'github-token' is not set. Please give API token to push GitHub pages

which looks like something I can't fix/disable on my end, and which conditioned me to ignore/not check for actual failures like the lint.

Anyway, looks like everything's passing now. Please let me know if I missed something--I have a few more small fixes in the queue ready to PR.

[t3chguy]

Great, thanks!