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

Support action buttons in UXDS #103

Merged
merged 5 commits into from
Nov 15, 2023
Merged

Support action buttons in UXDS #103

merged 5 commits into from
Nov 15, 2023

Conversation

maxatdetroit
Copy link
Member

Part of #100

This PR

  1. Creates a icon container for using icons within cards
  2. Creates a higher-order element <clickable/> that causes child elements to behave as a clickable element
  3. Adds funtionality to the card element to become a clickable component

Alternatives

Instead of introducing Clickable, we could have:

  1. Extended or re-used the cod-button component. The main reason to avoid this approach is that some elements should not be considered buttons for accessibility (e.g. a screenreader should treat action buttons as links).
  2. Built another standalone component. The main reason to avoid this approach is that we miss out on composability of components (one of the main pros of a UXDS). E.g. we can re-use <cod-clickable/> and <cod-card/> when building arpa buttons.

Testing

  1. Build a couple stories that render clickable cards (styled as action buttons).
  2. View them in storybook.
Screenshot 2023-10-05 at 5 59 36 PM Screenshot 2023-10-05 at 5 59 32 PM Screenshot 2023-10-05 at 5 59 27 PM Screenshot 2023-10-05 at 5 59 17 PM

Note: The action buttons are not equally sized in the test but can be by placing them in a card grid (like we do on the city homepage).

  1. Observe cards in storybook to make sure there are no visual regressions.
Screenshot 2023-10-05 at 6 11 41 PM Screenshot 2023-10-05 at 6 11 37 PM
  1. Run test suite
$ yarn test-storybook
...

Test Suites: 31 passed, 31 total
Tests:       157 passed, 157 total
Snapshots:   0 total
Time:        9.096 s
Ran all test suites.

@maxatdetroit maxatdetroit requested a review from jedgar1mx October 5, 2023 22:14
@maxatdetroit maxatdetroit self-assigned this Oct 5, 2023
@maxatdetroit maxatdetroit added the enhancement New feature or request label Oct 5, 2023
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.

This might cause some ADA flags since there is no valid text or description for the link.

@maxatdetroit
Copy link
Member Author

Good catch @jedgar1mx , let me look at how we can better support ADA while still keeping the higher order component.

@maxatdetroit
Copy link
Member Author

maxatdetroit commented Oct 16, 2023

@jedgar1mx this approach satisfies ARIA roles and other accessibility API metadata quite well. See details below and please let me know if I missed something related to a11y, and I'll go back to correct it.

Details

role, name, action, state, and value are all appropriately set for the action button example presented in the test plan. Here's a snapshot of the accessibility tree using Firefox Accessibility Inspector:

{
    "name": "Do Something Like click on this card.",
    "role": "link",
    "actions": [
        "Jump"
    ],
    "value": "https://example.com/",
    "nodeCssSelector": ".btn",
    "nodeType": 1,
    "description": "",
    "keyboardShortcut": "",
    "childCount": 1,
    "indexInParent": 0,
    "states": [
        "focusable",
        "linked",
        "traversed",
        "opaque",
        "enabled",
        "sensitive"
    ]
}

Screenshot 2023-10-16 at 12 36 30 PM

Note: The icons used in the action button are missing titles, but that should be address in a separate PR (a fix put up to set alt for <cod-icon> SVGs).

@maxatdetroit
Copy link
Member Author

@jedgar1mx merge conflicts are resolved here and ready for review.

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.

Is CardIconContainer a necessary component? It seems that we could just add the Icon on the CardHeader or CardBody as just part of the mark up.

@maxatdetroit
Copy link
Member Author

Probably not necessary to have a separate component for CardIconContainer. It was originally intended to encapsulate the styles, but after a few iterations, the styles went down to one line (the top margin above the icon) so let me go ahead and inline the icon as markup in the header/body.

@maxatdetroit maxatdetroit marked this pull request as draft November 14, 2023 14:06
@maxatdetroit maxatdetroit marked this pull request as ready for review November 14, 2023 22:41
@jedgar1mx jedgar1mx merged commit 5725f5e into dev Nov 15, 2023
3 checks passed
@jedgar1mx jedgar1mx deleted the feature.button.100 branch November 15, 2023 14:45
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