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

Create action button component #158

Merged
merged 5 commits into from
Feb 16, 2024
Merged

Create action button component #158

merged 5 commits into from
Feb 16, 2024

Conversation

maxatdetroit
Copy link
Member

@maxatdetroit maxatdetroit commented Feb 12, 2024

Context

See context in #100.

This PR

  • Create a new component for action button
  • Create story for action button with various args
  • Remove story for cod-clickable component since it is for internal use only

Testing

Test new story with various args:

story-test
image

Check chromatic for no regression of other components

@maxatdetroit maxatdetroit added the enhancement New feature or request label Feb 12, 2024
@maxatdetroit maxatdetroit self-assigned this Feb 12, 2024
Copy link
Member

@jedgar1mx jedgar1mx left a comment

Choose a reason for hiding this comment

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

Also, it would be good to take into account how they interact as a group since that's usually how they are implemented.

image

aButton.setAttribute('btn-color', args.buttonColor);
aButton.setAttribute('icon', args.icon);
aButton.setAttribute('title', args.title);
aButton.setAttribute('body', args.body);
Copy link
Member

Choose a reason for hiding this comment

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

It might be better to setup the component using slots rather than attributes. It will probably make the implementation easier. The body field on drupal will export mark up.

Copy link
Member Author

Choose a reason for hiding this comment

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

@jedgar1mx , I intentionally used an attribute here because that will force users of the button to not try and shove too much inside the button. These buttons aren't really designed to have rich, formatted content inside. The 'body' is meant to be a simple sentence or few words.

With that in mind, let me know if you still think slots is a better approach here.

Copy link
Member

Choose a reason for hiding this comment

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

Using slot for the body field is probably a better approach to accommodate the old content.

@maxatdetroit
Copy link
Member Author

maxatdetroit commented Feb 14, 2024

@jedgar1mx I fixed the multi-button layout issue.

image

I also went ahead and scrapped the idea of the Clickable component. I think a sound approach for deciding whether to nested components is the following:

  • When to build from vanilla HTML, CSS, bootstrap:
    • When building a standalone, reusable component. E.g. a button.
  • When to compose components:
    • When building a reusable interface where the nested component styles are independent. E.g. a button group or customizable form with some validation logic.
    • Out of necessity when UXDS users must slot in rich markup. E.g. card body is nested within a card. Note: Even here I think there is a case to remove internal components like card body or table body and have the user use named slots instead of a custom nested component.

The reason for this shift in mindset: it's overly cumbersome and also fragile to have components control one another's styles. Components that are reused often will quickly devolve into unmaintainable style sheets. Also, composing components that control each other's styles seems like an anti-pattern with shadow DOM style encapsulation.

@maxatdetroit maxatdetroit marked this pull request as draft February 14, 2024 20:47
@maxatdetroit
Copy link
Member Author

Moved this back to draft while I:

  • Make the body slottable
  • Fix alignment of content when in grid layout

@maxatdetroit maxatdetroit marked this pull request as ready for review February 14, 2024 21:42
@maxatdetroit
Copy link
Member Author

@jedgar1mx full markup is now supported in the body (added a story for demo), also fixed the alignment in grid layouts.

Ready to review.

@jedgar1mx jedgar1mx merged commit 0b3b2f2 into dev Feb 16, 2024
5 checks passed
@jedgar1mx jedgar1mx deleted the feature.100.actionbutton branch February 16, 2024 13:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants