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

v5 component refactor: Buttons #140

Closed
10 of 11 tasks
HollyJoyPhillips opened this issue Jul 30, 2024 · 6 comments · Fixed by #179
Closed
10 of 11 tasks

v5 component refactor: Buttons #140

HollyJoyPhillips opened this issue Jul 30, 2024 · 6 comments · Fixed by #179
Assignees
Labels
feature The issue relates to a new feature v5 Issues for v5 release

Comments

@HollyJoyPhillips
Copy link

HollyJoyPhillips commented Jul 30, 2024

Abstract

As part of the v5 Elements release, each component will be reviewed and refactored to ensure best practice and design system alignment

Specification

Developer Checklist

  • Styles alignment between Design System and Elements
  • Check design tokens in Figma and implement CSS variable tokens if available for relevant component
  • Align with accessibility standards / spec as per above
  • If relevant, break down component into Styles Only and React component structures
  • Ensure all variants of components are documented as appropriate
  • Ensure unit test coverage is adequate for component
  • Update documentation in MDX file as per guidelines
  • Changelog updated to reflect a single beta version per component ideally

Release Checklist

  • Approved PR merged to main
  • Design & product review and feedback addressed by developer
  • Beta release by product / engineering lead to next beta version

Additional Context or Information

@HollyJoyPhillips HollyJoyPhillips added feature The issue relates to a new feature v5 Issues for v5 release labels Jul 30, 2024
@ksolanki7 ksolanki7 self-assigned this Oct 14, 2024
@ksolanki7
Copy link
Contributor

Hello Team,

I have reviewed this ticket's requirements and summarized my understanding below. Please review and let me know if I've missed anything or if adjustments are needed to my approach.

Updates to the Button Component:

Since the Button component already exists, the necessary updates will be applied directly to it. The required changes are as follows:

1. UI Updates:

  • Adjust the padding, font size, and other styles according to the design system (DS).

2. New Variants for the Button Component:

intent: (Should we retain this variant name as "intent," or has the team discussed a new name?)

  • Primary
  • Secondary (default)
  • Tertiary
  • Destructive
  • Busy

state:

  • Default
  • Focused
  • Hovered
  • Disabled

size:

  • Small
  • Medium
  • Large

iconLeft
iconRight

Additionally, I will be creating new exports for the SplitButton and ReversedButton components. These will be used as shown below, incorporating the relevant variants from the DS:

import { SplitButton, ReversedButton } from '@reapit/elements'

return (
  <SplitButton>Button</SplitButton>
  <ReversedButton>Button</ReversedButton>
)

@HollyJoyPhillips
Copy link
Author

Hi @ksolanki7, I've reached out to the designers for comment. Thanks.

@kurtdoherty
Copy link
Contributor

kurtdoherty commented Oct 15, 2024

suggestion: Personally, I'd prefer to see us use variant or something similar over intent. I don't think intent communicates the same concept. Primary, secondary, tertiary etc, are different variants/variations of a Button; they aren't indicating intent.

suggestion: I would descope SplitButton and ReversedButton from this ticket. They are novel enough (the split buttons espectially) to warrant their own tickets/design docs.

suggestion: It could be worthwhile discussing whether disabled buttons should leverage the disabled attribute or not. This article provides a good summary of the a11y concerns with disabled buttons and links out to additional reading on the topic. In the Console Cloud component library, we took the more accessible approach (aria-disabled instead of disabled), but that decision pre-dates the current Design team, so I'm not sure what their opinion is on this topic.

suggestion: When we talk about the Button component in the Design System, we're talking about a visual element, not a DOM element. As engineers, we know not all visual buttons in the UI will actually be <button> elements in the DOM; some will need to be <a> elements such that users are navigated to another page/URL when the visual button is clicked. Likewise, not all visual links will be <a> elements; some will be <button> elements.

I think we need to decide how we'll handle this in Elements, which (in my opinion) is nuanced enough to warrant some kind of design doc for us to discuss.

@ksolanki7
Copy link
Contributor

praise: Thank you, @kurtdoherty, for the valuable feedback.

note: While we await a response from the UI team, I’ll begin working on the core UI aspect of the button component to ensure alignment with the design system (DS).

note: For now, I will descope SplitButton and ReversedButton from this ticket.

thought: I suggest we arrange a kick-off or handover meeting with the dev, UI, and product teams for every v5 refactor or new component ticket to align expectations and ensure a smooth process.

@HollyJoyPhillips
Copy link
Author

Hi @ksolanki7,

All good from the design team. Only comment:

"I think we refrain from using “intent” with buttons. True - they are intentional - but we apply buttons based on importance in the hierarchy."

ksolanki7 added a commit that referenced this issue Oct 30, 2024
* refactor: #140 part 2 v5 ButtonGroup component

* Fix issue with test cases

* Fix issue with page header button props

* Fix issue with file input test case

* Fix issue with button-group test case

* Added additional test cases to increase the test coverage

* Updated test cases to support updated parent PR
ksolanki7 added a commit that referenced this issue Nov 1, 2024
* refactor: #139 v5 ButtonGroup component

* PR feedback update - Replace boolean variants to enum, fix typescript for anchor v/s button, replace mandate elements Icon usage to reactNode

* Fix issue with test case and removed onClick from anchor

* Reintroduce onClick event on anchor element

* Final feedback touchup

* refactor: #140 part2 v5 component refactor buttons (#180)

* refactor: #140 part 2 v5 ButtonGroup component

* Fix issue with test cases

* Fix issue with page header button props

* Fix issue with file input test case

* Fix issue with button-group test case

* Added additional test cases to increase the test coverage

* Updated test cases to support updated parent PR

* Fix issue with Type test

* Updated menu to support new button component

* Updated PR with feedback from Design team

* Added Change log

* Fix snapshot test
@github-project-automation github-project-automation bot moved this from In Progress to Done in Elements Roadmap Nov 1, 2024
@ksolanki7
Copy link
Contributor

@HollyJoyPhillips The Button component refactor dev work is complete now and release to beta environment.

Note: I have gone through initial review with Andrei from Design team over my local environment before pushing it to beta environment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature The issue relates to a new feature v5 Issues for v5 release
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants