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

Draft ToggleGroup docs & examples #3883

Open
wants to merge 31 commits into
base: master
Choose a base branch
from
Open

Draft ToggleGroup docs & examples #3883

wants to merge 31 commits into from

Conversation

taysea
Copy link
Collaborator

@taysea taysea commented May 24, 2024

Deploy Preview

What does this PR do?

Scaffold page for ToggleGroup docs.

TO DO:

  • Fix tabindex on individual togglegroup buttons in preview

Where should the reviewer start?

What testing has been done on this PR?

In addition to the feature you are implementing, have you checked the following:

General UX Checks

  • Small, medium, and large screen sizes
  • Cross-browsers (FireFox, Chrome, and Safari)
  • Light & dark modes
  • All hyperlinks route properly

Accessibility Checks

  • Keyboard interactions
  • Screen reader experience
  • Run WAVE accessibility plugin (Chrome)

Code Quality Checks

  • Console is free of warnings and errors
  • Passes E2E commit checks
  • Visual snapshots are reasonable

How should this be manually tested?

Any background context you want to provide?

What are the relevant issues?

Screenshots (if appropriate)

Should this PR be mentioned in Design System updates?

Is this change backwards compatible or is it a breaking change?

@taysea taysea mentioned this pull request May 24, 2024
8 tasks
@vavalos5
Copy link
Collaborator

@taysea the deploy link doesn't seem to be working.

@taysea
Copy link
Collaborator Author

taysea commented May 31, 2024

@taysea the deploy link doesn't seem to be working.

Added the PR # to the link, should work now.

britt6612 and others added 2 commits June 10, 2024 09:11
Added generic examples to use cases.

Removed Type as a heading because it was the same content as use cases.
@SOjaHPE
Copy link
Collaborator

SOjaHPE commented Jun 11, 2024

@vavalos5 - some feedback for your posted preview:
Description text - revisit this, make sure to double check spelling/grammar before submitting, not sure if the following description text has too much detail, "A group of related buttons that allow users to select one or multiple options from a closely positioned set, emulating the behavior of radio buttons for single selection and checkboxes for multiple selections." The point is that we want to make sure it provides enough information to aid a user with finding the right component for their use case.

Under the Types: Single Select section, currently mixes different conceptual ideas, which may confuse users. ‘Icon Only’ and ‘Label and Icon’ describe the visual styling of toggle buttons, whereas ‘Filtering’ describes their functional use case. To avoid confusion, it would be clearer to separate visual styling options from functional use cases. For instance, ‘Icon Only’ and ‘Label and Icon’ should be covered by the anatomy subsection, while ‘Filtering’ should be covered under a ‘Use Cases’ subsection. This distinction helps users understand that styling options define how toggle buttons look, whereas use cases define how they function.

Further, isn't it true that those styling options apply to both types? they were just listed under Single Select.

In addition, a more appropriate descriptor for the toggle function that switches between different views, such as a tabular layout and a map view, in a data collection set would be "view toggle" not Filtering. Those terms accurately reflect the functionality of switching between different visual representations of the same data.

@britt6612
Copy link
Collaborator

britt6612 commented Jun 12, 2024

@SOjaHPE you are wanting the description for the component to be
A group of related buttons that allow users to select one or multiple options from a closely positioned set, emulating the behavior of radio buttons for single selection and checkboxes for multiple selections ?

I was thinking a little more broader if we decouple this from CheckBoxGroup and RadioButtonGroup. Since you can have a controlled example using multiple but still enforcing one selection. They are also not build with CheckBoxGroup or RadioButtonGroup. They are a group of related buttons that allow users to turn off and on. Also we will catch spelling mistakes what reviews are for 💪

☝️ we can add something in the sense of in single allows the user to turn only one on then multiple more can be turned on and off. Its just they wont always have the native RadioButtonGroup behavior or CheckBoxGroup so my thoughts were not to add those. @taysea any thoughts here?

@britt6612
Copy link
Collaborator

britt6612 commented Jun 12, 2024

Under the Types: Single Select section, currently mixes different conceptual ideas, which may confuse users. ‘Icon Only’ and ‘Label and Icon’ describe the visual styling of toggle buttons, whereas ‘Filtering’ describes their functional use case. To avoid confusion, it would be clearer to separate visual styling options from functional use cases. For instance, ‘Icon Only’ and ‘Label and Icon’ should be covered by the anatomy subsection, while ‘Filtering’ should be covered under a ‘Use Cases’ subsection. This distinction helps users understand that styling options define how toggle buttons look, whereas use cases define how they function.

agree here we can have something in the anatomy that shows how you can put an icon or label or both. Then in use cases we can have TextEditor example as well as others

Further, isn't it true that those styling options apply to both types? they were just listed under Single Select.

Yes this is ture

In addition, a more appropriate descriptor for the toggle function that switches between different views, such as a tabular layout and a map view, in a data collection set would be "view toggle" not Filtering. Those terms accurately reflect the functionality of switching between different visual representations of the same data.

not sure Im following here ☝️

@SOjaHPE
Copy link
Collaborator

SOjaHPE commented Jun 12, 2024

Under the Types: Single Select section, currently mixes different conceptual ideas, which may confuse users. ‘Icon Only’ and ‘Label and Icon’ describe the visual styling of toggle buttons, whereas ‘Filtering’ describes their functional use case. To avoid confusion, it would be clearer to separate visual styling options from functional use cases. For instance, ‘Icon Only’ and ‘Label and Icon’ should be covered by the anatomy subsection, while ‘Filtering’ should be covered under a ‘Use Cases’ subsection. This distinction helps users understand that styling options define how toggle buttons look, whereas use cases define how they function.

agree here we can have something in the anatomy that shows how you can put an icon or label or both. Then in use cases we can have TextEditor example as well as others

Further, isn't it true that those styling options apply to both types? they were just listed under Single Select.

Yes this is ture

In addition, a more appropriate descriptor for the toggle function that switches between different views, such as a tabular layout and a map view, in a data collection set would be "view toggle" not Filtering. Those terms accurately reflect the functionality of switching between different visual representations of the same data.

not sure Im following here ☝️

@britt6612 In the current guidance that @vavalos5 posted, under the Types: Single select section, the third example uses the term 'Filtering' when referencing the toggling between data collection view types. Just pointing out that filtering doesn't accurately reflect that use case, it is 'toggling views' or 'view toggle' for the appropriate data collection type.

@vavalos5
Copy link
Collaborator

vavalos5 commented Jun 12, 2024

Under the Types: Single Select section, currently mixes different conceptual ideas, which may confuse users. ‘Icon Only’ and ‘Label and Icon’ describe the visual styling of toggle buttons, whereas ‘Filtering’ describes their functional use case. To avoid confusion, it would be clearer to separate visual styling options from functional use cases. For instance, ‘Icon Only’ and ‘Label and Icon’ should be covered by the anatomy subsection, while ‘Filtering’ should be covered under a ‘Use Cases’ subsection. This distinction helps users understand that styling options define how toggle buttons look, whereas use cases define how they function.

agree here we can have something in the anatomy that shows how you can put an icon or label or both. Then in use cases we can have TextEditor example as well as others

Further, isn't it true that those styling options apply to both types? they were just listed under Single Select.

Yes this is ture

In addition, a more appropriate descriptor for the toggle function that switches between different views, such as a tabular layout and a map view, in a data collection set would be "view toggle" not Filtering. Those terms accurately reflect the functionality of switching between different visual representations of the same data.

not sure Im following here ☝️

@britt6612 In the current guidance that @vavalos5 posted, under the Types: Single select section, the third example uses the term 'Filtering' when referencing the toggling between data collection view types. Just pointing out that filtering doesn't accurately reflect that use case, it is 'toggling views' or 'view toggle' for the appropriate data collection type.

@taysea Would be great to get your input here since you and I discussed nesting these items under Filtering. Originally under "use cases" which I agree would live here.

"Use cases" are the difference kinds of scenarios where ToggleGroup might appear and "types" are the overall kinds of ToggleGroups there are: single select and multi-select. Happy to set a call to discuss naming here.

I seem to have swapped them.

@britt6612
Copy link
Collaborator

britt6612 commented Jun 12, 2024

Under the Types: Single Select section, currently mixes different conceptual ideas, which may confuse users. ‘Icon Only’ and ‘Label and Icon’ describe the visual styling of toggle buttons, whereas ‘Filtering’ describes their functional use case. To avoid confusion, it would be clearer to separate visual styling options from functional use cases. For instance, ‘Icon Only’ and ‘Label and Icon’ should be covered by the anatomy subsection, while ‘Filtering’ should be covered under a ‘Use Cases’ subsection. This distinction helps users understand that styling options define how toggle buttons look, whereas use cases define how they function.

agree here we can have something in the anatomy that shows how you can put an icon or label or both. Then in use cases we can have TextEditor example as well as others

Further, isn't it true that those styling options apply to both types? they were just listed under Single Select.

Yes this is ture

In addition, a more appropriate descriptor for the toggle function that switches between different views, such as a tabular layout and a map view, in a data collection set would be "view toggle" not Filtering. Those terms accurately reflect the functionality of switching between different visual representations of the same data.

not sure Im following here ☝️

@britt6612 In the current guidance that @vavalos5 posted, under the Types: Single select section, the third example uses the term 'Filtering' when referencing the toggling between data collection view types. Just pointing out that filtering doesn't accurately reflect that use case, it is 'toggling views' or 'view toggle' for the appropriate data collection type.

@SOjaHPE gotcha yeah I agree there with filtering just was a little confused about the wording here:
A group of related buttons that allow users to select one or multiple options from a closely positioned set, emulating the behavior of radio buttons for single selection and checkboxes for multiple selections.

Copy link
Collaborator

@britt6612 britt6612 left a comment

Choose a reason for hiding this comment

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

Looks good!

@vavalos5 vavalos5 linked an issue Jun 14, 2024 that may be closed by this pull request
8 tasks
@britt6612 britt6612 changed the title Draft ToggleGroup docs Draft ToggleGroup docs & examples Jun 14, 2024
@britt6612 britt6612 self-assigned this Jun 14, 2024
@britt6612 britt6612 requested a review from jcfilben June 17, 2024 16:19
<DataSummary />
{value === 'table' && (
<DataTable
aria-describedby="spaceX"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
aria-describedby="spaceX"

It doesn't look like any element has this id, should we remove?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I can remove but it was throwing an error

Copy link
Collaborator

Choose a reason for hiding this comment

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

What error do you get?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Screen Shot 2024-06-17 at 3 51 19 PM

Copy link
Collaborator

Choose a reason for hiding this comment

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

I can disable

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we can just add some text with the id that describes the datatable? Looks like that is the intent of the rule
https://github.com/grommet/eslint-plugin-grommet/blob/master/docs/rules/datatable-aria-describedby.md

Copy link
Collaborator

Choose a reason for hiding this comment

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

okay I can add

Copy link
Collaborator

Choose a reason for hiding this comment

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

done

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.

Documentation - Toggle Group
5 participants