Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revise SelectPanel documentation for new version #708

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

dipree
Copy link
Contributor

@dipree dipree commented Jan 11, 2024

Preparing the select panel documentation for the new implementation.

  • Streamline content to be less elaborate and easier to consume.
  • Update the structure for best readability. Guidelines here and here are currently a little inconsistent. Going with what's adopted in most components:
    • Overview
    • Anatomy
    • Options
    • Behaviors (also called Interactions)
    • Usage guidelines (we don't have that in the guidelines)
    • Accessibility
  • Remove redundant content unless it helps to explain accessibility best practices.

Latest deployment

src="https://github.com/primer/design/assets/980622/f2867468-4606-4999-8ea7-21860e50051d"
/>

Select panels can be activated by either a regular [button](/components/button), [icon button](/components/icon-button) or [action menu item](/components/action-menu). If the button is intended to represent the current selection, it's crucial to have a label associated with it, either internally (within the button) or externally (adjacent to the button).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clarifying question: Are these intended as examples or a closed set?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@siddharthkp good question, I didn't change anything to the existing content here, just condensed it a little. I assume that it was meant to be exclusive to these. Is there anything else that is missing here?

@@ -190,32 +220,6 @@ By employing these loading states, users can have a better understanding of the
</Dont>
</DoDontContainer>

### Click-to-dismiss behavior
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👀 Should we keep this section until we get consensus and remove it?

(Side note: Should change the title to "Click-outside-to-dismiss behavior)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of removing this section completely, should we replace this with the behavior we want now?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm okay with leaving this out. What we're going with is the true and expected nature of a dialog, which is covered in the "Closing" section of this doc. (we should put the videos back in though)

Copy link
Member

@siddharthkp siddharthkp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some clarifying questions, approving in advance though

@dipree dipree marked this pull request as ready for review February 14, 2024 09:47
@dipree dipree requested a review from a team as a code owner February 14, 2024 09:47
content/components/selectpanel.mdx Outdated Show resolved Hide resolved
content/components/selectpanel.mdx Show resolved Hide resolved
@ichelsea
Copy link
Contributor

I added myself as a reviewer here; looks like a quite a bit of changes from what I originally wrote and then worked with @maximedegreve on revising.

I'd like to get this reviewed today and hoping we can hold off merging until next week on this one.

content/components/selectpanel.mdx Outdated Show resolved Hide resolved
content/components/selectpanel.mdx Outdated Show resolved Hide resolved
content/components/selectpanel.mdx Outdated Show resolved Hide resolved
content/components/selectpanel.mdx Outdated Show resolved Hide resolved
dipree and others added 2 commits February 20, 2024 08:59
Co-authored-by: Maxime De Greve <maximedegreve@github.com>

<Note>

When the select panel is activated through an [action menu](/components/action-menu), it should be displayed as a centered dialog using the `dialog` property.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it should be displayed as a centered dialog using the dialog property.

are you referring to the modal variant here? (not dialog)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@siddharthkp that sounds right, because they're all dialogs.

@@ -309,7 +296,7 @@ Items accompanied by a checkmark icon, as opposed to radio buttons or checkboxes
</Dont>
</DoDontContainer>

When an additional checkbox is added to the footer, declarative buttons are required, and the items should be accompanied by visually resembling radio buttons. Although they are not technically radio buttons in terms of semantics, using visual radio buttons can effectively convey that the selection process is not yet complete even after making a choice.
Adding a checkbox to the footer requires declarative buttons, and items should feature visual radio buttons. While not actual radio buttons semantically, this design effectively indicates that the selection process continues after a choice is made.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Not related to just this PR) Worth mentioning that this is only true for single selection, right? (not multiple selection, because radio)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep!

Copy link
Member

@joshblack joshblack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking the time to clean things up and clarify! Just left some comments/questions 👀

@@ -31,31 +21,15 @@ It's important to understand the differences between the select panel and [actio
src="https://github.com/primer/design/assets/980622/d396ea70-d8a5-4e52-ae3e-7596a3ef39b6"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One note for this image, declaritive -> declarative


We highly recommend utilizing the search functionality instead of retrieving extensive lists from our API endpoints to enable the select all feature. By doing so, you can avoid potential performance issues, especially considering that this component lacks virtualization capabilities.
The indeterminate selections feature is valuable for bulk actions on lists. For instance, if you have 20 selected items and some of them are tagged with different labels, an indeterminate checkbox (indicated by a horizontal line) appears for labels that are not uniformly applied to all items.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From a technical perspective, wanted to ask how do we support indeterminate states on an option in listbox?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know that we can support a true indeterminate state. Things in listbox are either selected or they're not. We'll likely have to do something like attach an aria-description to the indeterminate items and determine if we want them in a select, unselected, or unselectable state.

cc: @maximedegreve


<AccessibilityLink label="SelectPanel"/>
Automatic saving may be suitable for single selections that do not include additional form controls. In such cases, the selection process is straightforward and does not require further user confirmation, making it accessible and efficient. However, when the selection process involves additional steps or consequences, explicit saving mechanisms are crucial to ensure that all users, including those with accessibility needs, can confidently and accurately manage their selections.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Random question, for the "instant selection" or simpler cases for selection without a filter, are these situations where we would prefer folks to use components other than a select panel? (like an actionlist or action menu directly)

Copy link
Contributor

@ichelsea ichelsea left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry this is lengthy but it's important 🙂

Search/filter

This was true prior to the edits but we refer to the input indeterminately as filter or search; we should probably stay consistent with one of them, whichever we pick.

(search is generally understood to look through a database that isn't visually presented in front of the user to bring them results that match. filter takes a visually existing dataset and narrows down the results for the user. In our case, we have a little bit of both in that we show users a partial data set but you can search for ones that aren't present in the list that we load)


For instance, you may encounter a select panel when you need to assign someone to a task or add labels to an issue or pull request. It can also be utilized to select a specific branch in a code repository.

It's important to understand the differences between the select panel and [action menu](/components/action-menu). The primary distinction lies in the fact that an action menu lacks the ability to include additional form controls and automatically closes after making a selection. Therefore, if you require a filterable list or the capability to make multiple selections at once, the select panel is the appropriate choice.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This paragraph should stay in the documentation, like @maximedegreve mentioned, it's important to keep this information as people still get confused. I think it's important to have in the overview section because it's the first thing people tend to look at to see if it fits their needs. The distinction still appears to be removed from the documentation.

For instance, you may encounter a select panel when you need to assign someone to a task or add labels to an issue or pull request. It can also be utilized to select a specific branch in a code repository.

It's important to understand the differences between the select panel and [action menu](/components/action-menu). The primary distinction lies in the fact that an action menu lacks the ability to include additional form controls and automatically closes after making a selection. Therefore, if you require a filterable list or the capability to make multiple selections at once, the select panel is the appropriate choice.
The select panel allows users to choose one or multiple items from one or more lists and can include search functionality to speed up the selection. It can also be enhanced with additional form controls in the footer. For instance, the select panel is ideal for assigning individuals to tasks, adding labels to issues or pull requests, or switching to specific branches in a code repository.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
The select panel allows users to choose one or multiple items from one or more lists and can include search functionality to speed up the selection. It can also be enhanced with additional form controls in the footer. For instance, the select panel is ideal for assigning individuals to tasks, adding labels to issues or pull requests, or switching to specific branches in a code repository.
The select panel allows users to select and choose one or multiple items and can include search functionality to speed up the selection. It can also be enhanced with additional form controls in the footer. For instance, the select panel is ideal for assigning individuals to tasks, adding labels to issues or pull requests, or switching to specific branches in a code repository.

I'm cautious about adding this part about one or more lists specifically as it feels like it's adding unnecessary cognitive overhead. I'm assuming it's there for when we have select panels with tabs, but it's still all about choosing one or more item. The tabs will be an option, I don't want to start seeing select panels with multiple lists to choose from on the same dialog panel as a concept from this.

Comment on lines +26 to +28
- **Header**: Always includes a title and a close button. An optional search/filter field can be added for quicker item finding. Once a selection is made, a "Deselect all" button should be visually present.
- **List (scrollable)**: Comprises [action list](/components/action-list) items, supporting both single and multi-select options.
- **Footer**: In multi-select scenarios, it includes "Cancel" and "Apply" buttons. If additional controls (like a checkbox or button) are needed, a footer is also required for single selections.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- **Header**: Always includes a title and a close button. An optional search/filter field can be added for quicker item finding. Once a selection is made, a "Deselect all" button should be visually present.
- **List (scrollable)**: Comprises [action list](/components/action-list) items, supporting both single and multi-select options.
- **Footer**: In multi-select scenarios, it includes "Cancel" and "Apply" buttons. If additional controls (like a checkbox or button) are needed, a footer is also required for single selections.
- **Header**: always includes a title and a close button. An optional search/filter field can be added to find items more quickly. Once a selection is made, a "Deselect all" button should be present visually within the input on the far right.
- **List (scrollable)**: contains [action list](/components/action-list) items, supporting single or multi-select options.
- **Footer**: is required for multi-select scenarios and includes declarative buttons (e.g., "Cancel" and "Apply". If additional controls (like a checkbox or button) are needed, a footer is also required for single selections.
  • Grammatical updates; updating the first two bullet points for sentence case means the third bullet point should also be sentence case.
  • I think this is important to indicate, specifically so that designers and engineers know it will be included and that it's only visually a part of the input because semantically it sits outside of the input.
  • Reworded the list section to indicate that you can use single or multi-select options but not both at the same time.
  • Added back in "declarative actions" because it's what is used to describe the footer confirmation buttons in the reference image.

Comment on lines -50 to -58
### Trigger

<img
width="960"
alt="Various trigger examples are displayed. The first example showcases an icon button with a cog icon. The second example presents a button with the internal label 'Assignees: 2 people'. The third example features a button with the external label 'Assignees' and an internal button label stating '2 people'. Lastly, the image shows an action menu with two items: 'Set labels...' and 'Delete...'."
src="https://github.com/primer/design/assets/980622/f2867468-4606-4999-8ea7-21860e50051d"
/>

Select panels can be activated by either a regular [button](/components/button), [icon button](/components/icon-button) or [action menu item](/components/action-menu). If the button is intended to represent the current selection, it's crucial to have a label associated with it, either internally (within the button) or externally (adjacent to the button).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would keep the trigger section here, as they are part of the options for building out select panel, and a select panel can't be built without a trigger. It helps the designer to keep the launch of the component consistent across experiences.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and a select panel can't be built without a trigger.

I'm not sure I understand the deliberation here. What would be the alternative, to render it on page load?

@@ -65,28 +39,27 @@ Select panels can be activated by either a regular [button](/components/button),
src="https://github.com/primer/design/assets/980622/928a4bb2-c3f8-4620-9e37-42d9b91204c1"
/>

If your selection doesn't trigger a series of events upon submission and doesn't include a checkbox in the footer, you may consider not adding a cancel/save button. In this scenario, when a selection is made, the select panel will instantly close similar to a menu. This behavior is indicated by a checkmark on the selected item.
For example switching a branch is easily reversible and doesn't pose any risk of sending notifications to other users.
For single-item selection, if no consequential actions follow, the panel may close immediately, indicated by a checkmark on the selected item. However, when the selection triggers significant actions or the footer has additional elements (like a checkbox), declarative buttons are necessary. In such cases, the behavior is indicated visually by radio buttons, which allows users to confirm their choices, before submitting their selection.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recommend keeping an example here of what a consequential and non-consequential (changing the viewed branch in a code tab) action would look like; vague descriptions in components make the hardest concepts to grasp.

@@ -366,7 +353,7 @@ The select panel offers multiple methods for clearing data, depending on the con
</Dont>
</DoDontContainer>

For multi-selections with search functionality, a convenient **deselect all** [icon button](/components/icon-button) becomes available as soon as at least one item is selected.
In multi-selections with search capabilities, a **deselect all** [icon button](/components/icon-button) is accessible for easy unselection as soon as one item is selected.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
In multi-selections with search capabilities, a **deselect all** [icon button](/components/icon-button) is accessible for easy unselection as soon as one item is selected.
In multi-selections with search functionality, a **deselect all** [icon button](/components/icon-button) becomes available for easy deselection as soon as one item is selected.

Comment on lines -392 to -413
### Action menu trigger

When the select panel is activated through an [action menu](/components/action-menu), it should be displayed as a modal dialog. It should never be shown as a submenu.

<DoDontContainer>
<Do>
<img
width="464"
alt="An action menu is presented with multiple items. The first item, 'Visible to...', is currently hovered, and it is annotated with 'Open as modal dialog'."
src="https://github.com/primer/design/assets/980622/22386756-2612-40dd-aceb-1f765095cf17"
/>
<Caption>When the select panel is triggered by an action menu, it should be presented as a modal dialog.</Caption>
</Do>
<Dont>
<img
width="464"
alt="A action menu with multiple items and where the first item 'Visible to' has a arrow on the right of the item and opened a multi select panel upon hover."
src="https://github.com/primer/design/assets/980622/a71c59d6-ca35-49ac-b082-e72aa4d1a8cd"
/>
<Caption>Avoid displaying the select panel as a non-modal submenu within an action menu.</Caption>
</Dont>
</DoDontContainer>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we removing this section? I have a feeling we'll get questions on what is possible if we don't include this.

Comment on lines -527 to -541
#### Example behavior

<Box display="grid" gridTemplateColumns={['1fr', null, null, null, '1fr 1fr']} gridGap={5}>
<div>
<CustomVideoPlayer
width="100%"
src="https://user-images.githubusercontent.com/40274682/228391029-44ab995b-7a04-4b85-825e-dc77f90838dd.mp4"
/>
<Caption>
Keyboard interaction with a single-select select panel demonstrating selection following focus with navigation via
the ArrowDown key.
</Caption>
</div>
</Box>

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you like me to re-record an updated version of this instead of removing it entirely? Selection following focus is often a confused concept.

## Accessibility

### Known accessibility issues (GitHub staff only)
For enhanced accessibility, it's important to use explicit saving mechanisms in the select panel. This includes incorporating "Apply" and "Cancel" buttons for both single- and multi-selection scenarios. These buttons provide clear, accessible actions for users to confirm or revoke their selections.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need this since the use case of declarative actions is already well covered in the document. This also mentions single select which confuses our guidance earlier in the document.

Suggested change
For enhanced accessibility, it's important to use explicit saving mechanisms in the select panel. This includes incorporating "Apply" and "Cancel" buttons for both single- and multi-selection scenarios. These buttons provide clear, accessible actions for users to confirm or revoke their selections.


<AccessibilityLink label="SelectPanel"/>
Automatic saving may be suitable for single selections that do not include additional form controls. In such cases, the selection process is straightforward and does not require further user confirmation, making it accessible and efficient. However, when the selection process involves additional steps or consequences, explicit saving mechanisms are crucial to ensure that all users, including those with accessibility needs, can confidently and accurately manage their selections.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is similar to my comment on the previous paragraph in that we don't need it because the declarative actions use cases are well covered prior to this. I also don't understand what this paragraph is trying to say, it's vague without examples and a bit lengthy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants