react icon indicating copy to clipboard operation
react copied to clipboard

Release SelectPanel accessibility improvements as a draft component

Open broccolinisoup opened this issue 2 years ago • 16 comments

SelectPanel introduced breaking behavioural changes in v35.8.0 and that caused issues for consumers. So we decided to rollback these changes (see the PR for ref) and release SelectPanel accessibility improvements as a draft component.

cc @siddharthkp @hectahertz @bolonio

broccolinisoup avatar Sep 15 '22 23:09 broccolinisoup

What were the breaking changes? I remember @hectahertz making the prop title optional to avoid breaking changes. Is there anything else?

bolonio avatar Sep 16 '22 07:09 bolonio

Hi @bolonio 👋🏼 it was more like a breaking behavioural changes as in introducing save and cancel buttons yet we received some feedback from memex team regarding some issues around it. Please see this issue for more.

This is why we decided to move the SelectPanel changes to a draft component and by doing that, we wouldn't be blocking our consumers while we are addressing the reported issues. Please let us know if you still have any concerns. 🙏🏼

broccolinisoup avatar Sep 16 '22 11:09 broccolinisoup

👋 Hi @broccolinisoup ~ What is the process for addressing breaking behavioral changes that were introduced to be accessible? We don't want to default to a less-accessible experience.

cc @lesliecdubs @howdyray

inkblotty avatar Sep 16 '22 14:09 inkblotty

Hi!

What is the process for addressing breaking behavioral changes that were introduced to be accessible?

(Only relevant for breaking changes) To make sure folks we don't block folks from getting fixes and features in new releases, the process is to keep two versions of a component in a non-breaking way, and import them from a different path:

// current component used on production:
import { SelectPanel } from '@primer/react'

// new accessible  component meant to replace current component:
import { SelectPanel } from '@primer/react/drafts'

We don't want to default to a less-accessible experience.

That's the bug/feature with this approach. Each instance of the current SelectPanel can be moved to the new SelectPanel one at a time, making sure it works and is accessible, while also keep up to date with other updates.

We've found this works better than requiring changes to multiple instances all at once because then folks get stuck with an old version of the library. (this recently came up in a discussion from gh/gh as well)

Unfortunately, for a some time, new instances of SelectPanel might accidentally be built on the old one, because that's the default. We solve this by adding a note on the docs to use the new version from drafts, but folks don't always look at the docs before creating a component.

In the next major version, we "promote" the new accessible Select panel to be the default, while still keeping the old version for teams that have not invested the time to upgrade.

// accessible SelectPanel, now the default
import { SelectPanel } from '@primer/react'

// old component still available under deprecated
import { SelectPanel } from '@primer/react/deprecated'

siddharthkp avatar Sep 16 '22 14:09 siddharthkp

Thank you @siddharthkp ! I appreciate the clear process here.

When it comes to evaluating the feature changes that are causing the "draft" state, what is the process?

inkblotty avatar Sep 16 '22 15:09 inkblotty

Hi everyone, I would like to give some context here about those "breaking behavioral changes that we introduced to the SelectPanel to be accessible".

We, @hectahertz and I, did the Primer React SelectPanel - A11y engineering review with the help of @jscholes, and there are a couple of things related with those "behavioral changes" I would like to clarify.

Before these changes, SelectPanel would call onSelectedChange every time the user would select or deselect an item.

The UX will change with the recommended layout, as there is a pair of Save and Cancel buttons. SelectPanel should call onSelectedChange only after the user performs a save, and discard the modifications altogether if the user cancels or closes the SelectPanel. We strongly discourage the addition of more granular callbacks, so we can keep the control of the pre-saving state local to the component. This approach also keeps the API virtually the same, so upgrades to the new version should be mostly straightforward.

  • Source: https://github.com/github/primer/issues/1021

The elements of the list within the SelectPanel act like checkboxes, both for single and multi selection. Checkboxes don't perform actions when checked, they just keep the selection until the save button is clicked.

Someone said in this comment in the memex / primer weekly sync, "this will change how users have to use this selector only in Memex (currently), which will be a frustrating user experience.".

We are aware that there are several places in dotcom where we have behaviours that we don't recommend, even if they could be frustrating for non-disabled users, but the component/feature/functionality would not be accessible.

I'm not against "promoting" the new accessible Select panel to be the default, while still keeping the old version for teams that have not invested the time to upgrade. But at the very end, every team will have to implement accessible components to have an accessible platform, and that would request some behavioral changes, since some of the current ones don't match with some of the principles of web accessibility.

As usual, we are always open for discussion, feel free to contact us in slack in #accessibility or attend our Accessibility Office Hours. We recommend you to book a slot in the session you're attending. You can do that searching for that day's session using this search link and adding yourself.

bolonio avatar Sep 19 '22 09:09 bolonio

Hi everyone, Sorry my wording on the ticket description was misleading. The main reason we need to revert the SelectPanel changes is due to our promise not introducing any breaking changes in the minor releases. (Also following Semver) However, mistakenly, we released SelectPanel changes with a minor release so we need to take this back to unblock folks and follow Semver standards. The rest is as Sid mentioned, we will release the SelectPanel with all the improvements in the next major release. Please see our list for the next major v36.x

Thank you for your understanding 🙏🏼 And as always, please let us know if you have any concerns.

broccolinisoup avatar Sep 19 '22 22:09 broccolinisoup

@broccolinisoup - This makes total sense. Thank you for clarifying, and let us know if you need the Accessibility Team to support.

inkblotty avatar Sep 20 '22 16:09 inkblotty

Thank you!! @inkblotty

broccolinisoup avatar Sep 20 '22 20:09 broccolinisoup

Thanks @broccolinisoup for the explanation :) As @inkblotty said and repeating myself:

As usual, we are always open for discussion, feel free to contact us in slack in #accessibility or attend our Accessibility Office Hours. We recommend you to book a slot in the session you're attending. You can do that searching for that day's session using this search link and adding yourself.

bolonio avatar Sep 21 '22 10:09 bolonio

Thanks everyone. Pinging @hectahertz here as a reminder when he's back from being out so we can chat about where this should sit in the priority queue.

lesliecdubs avatar Sep 26 '22 23:09 lesliecdubs

I'll leave here a link to a related discussion in the following issue:

  • https://github.com/primer/react/issues/2281

bolonio avatar Sep 27 '22 15:09 bolonio

Hi! This issue has been marked as stale because it has been open with no activity for 180 days. You can comment on the issue or remove the stale label to keep it open. If you do nothing, this issue will be closed in 7 days.

github-actions[bot] avatar Mar 26 '23 16:03 github-actions[bot]

Keeping this issue open until we release the remediations in the next major release

broccolinisoup avatar Mar 26 '23 23:03 broccolinisoup

Hi! This issue has been marked as stale because it has been open with no activity for 180 days. You can comment on the issue or remove the stale label to keep it open. If you do nothing, this issue will be closed in 7 days.

github-actions[bot] avatar Sep 23 '23 00:09 github-actions[bot]

@siddharthkp is this issue still relevant to the select panel work or not so much anymore?

broccolinisoup avatar Sep 24 '23 23:09 broccolinisoup