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

Feat(web, web-react): Introduce ActionGroup component #DS-1607 #1832

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

Conversation

pavelklibani
Copy link
Contributor

Description

Additional context

Issue reference

Component ActionGroup | Web

@pavelklibani pavelklibani self-assigned this Dec 30, 2024
@github-actions github-actions bot added the feature New feature or request label Dec 30, 2024
Copy link

netlify bot commented Dec 30, 2024

Deploy Preview for spirit-design-system ready!

Name Link
🔨 Latest commit 5526c01
🔍 Latest deploy log https://app.netlify.com/sites/spirit-design-system/deploys/677d19c5ef2a3f000889abd7
😎 Deploy Preview https://deploy-preview-1832--spirit-design-system.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
1 paths audited
Performance: 96 (no change from production)
Accessibility: 91 (no change from production)
Best Practices: 100 (no change from production)
SEO: 91 (no change from production)
PWA: -
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Dec 30, 2024

Deploy Preview for spirit-design-system-storybook canceled.

Name Link
🔨 Latest commit 5526c01
🔍 Latest deploy log https://app.netlify.com/sites/spirit-design-system-storybook/deploys/677d19c5d73967000856d2ce

@pavelklibani pavelklibani force-pushed the feat/ds-1607-action-group branch from 20a0d80 to c01dc94 Compare January 6, 2025 18:48
@pavelklibani pavelklibani marked this pull request as ready for review January 6, 2025 18:48
@pavelklibani pavelklibani changed the title Feat(web): Introduce ActionGroup component #DS-1607 Feat(web, web-react): Introduce ActionGroup component #DS-1607 Jan 6, 2025
@pavelklibani pavelklibani changed the title Feat(web, web-react): Introduce ActionGroup component #DS-1607 Feat(web, web-react): Introduce ActionGroup component #DS-1607 Jan 6, 2025
<div class="docs-Stack docs-Stack--stretch">

<div class="ActionGroup ActionGroup--row">
<a href="#" role="button" class="Button Button--primary Button--medium">Primary Action</a>
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
<a href="#" role="button" class="Button Button--primary Button--medium">Primary Action</a>
<a href="#" class="Button Button--primary Button--medium">Primary Action</a>

Please avoid using role="button", using the <button> element is more appropriate in such case.

Copy link
Contributor

Choose a reason for hiding this comment

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

We agreed in person to:

  1. not implement the ActionGroup component in the web package at all,
  2. instead, extend the Flex component:
    1. extend the Direction Dictionary with DirectionExtended which extends Direction with horizontalReversed and verticalReversed,
    2. deprecate row and column layouts of the Flex component in favor of the DirectionExtended dictionary, create codemod in React,
  3. in React, reuse the updated Flex component to create the ActionGroup component.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just for the naming. Isn't the Orientation with the values vertical/horizontal better than Direction? I think the "direction" suggests more from/to path than "orientation", which I understand as position on some point, e.g. a 360-degree circle (I hope this is understandable).

Copy link
Collaborator

Choose a reason for hiding this comment

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

issue (blocking): We have nothing to say here?

Copy link
Contributor

Choose a reason for hiding this comment

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

@pavelklibani left the README intentionally empty because he anticipated we would decide to discard the web component.


const ColumnLayout = () => {
return (
<ActionGroup direction="column">
Copy link
Collaborator

Choose a reason for hiding this comment

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

suggestion (blocking): I am not quite satisfied with the naming. I expect the direction is from left to right, up and down, not column or row.

Maybe orientation as I saw it in Adobe Spectrum Button Group would sound better. Of course with values like horizontal or vertical. And I think horizontal-reversed or vertical-reversed work as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

The thing is we currently use direction everywhere, it comes from our Direction dictionary. I am certainly open to a discussion, just be aware it affects all components with this prop.

<div class="docs-Stack docs-Stack--stretch">

<div class="ActionGroup ActionGroup--row">
<a href="#" role="button" class="Button Button--primary Button--medium">Primary Action</a>
Copy link
Collaborator

Choose a reason for hiding this comment

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

suggestion: Maybe @adamkudrna wants to suggest this:

Suggested change
<a href="#" role="button" class="Button Button--primary Button--medium">Primary Action</a>
<button class="Button Button--primary Button--medium">Primary Action</a>

Copy link
Contributor

Choose a reason for hiding this comment

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

Either of these, they are both correct, it doesn't matter. Just let's not use <a role="button"> 🙂.

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

Successfully merging this pull request may close these issues.

4 participants