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

Refactor React Button #468

Closed
anicholls opened this issue Feb 13, 2020 · 1 comment · Fixed by #471
Closed

Refactor React Button #468

anicholls opened this issue Feb 13, 2020 · 1 comment · Fixed by #471
Assignees
Labels
4.x enhancement New feature or request

Comments

@anicholls
Copy link
Contributor

anicholls commented Feb 13, 2020

Background

As our button component has grown, it has become increasingly complex. It has reached a point where any changes or improvements are extremely hard to make. Bug fixes often turn into a game of whackamole - where changing a style in one place for one use case can ripple into other use cases. There also has been a lot of discussion about separating the inner workings of IconButton from the regular button.

In order to support theming within button, we will likely require this refactor.

I'm not sure exactly what this refactor should look like yet, but I will update this issue as I do more research/exploration. To start, we will need to add a static states table to ensure no regressions are introduced by the updates.

Notes

  • Dropdown button should only allow variants primary and secondary
  • Dropdown button should not allow icon and dataLabel
  • Delete button should not allow icon or dataLabel
  • Small buttons should not allow icon or dataLabel
  • We should update the button build script to babel 7
  • Move IconButtonToggleGroup into a separate module called SegmentedControls and add support for text

Questions

  • Should we have a separate component for different variants? It's hard to enforce illegal prop combinations when everything is unified (i.e. <Button>, <HighlightButton>, etc.)
  • Should small buttons allow for icons and data text? If not, are these just hidden at this size? Should there be a small dropdown button? Right now we show icons for small text buttons but not other small buttons.
  • Should IconButton be a separate module? - no
@anicholls anicholls added this to Backlog in Current Sprint (7/20 - 8/9) via automation Feb 13, 2020
@anicholls anicholls added enhancement New feature or request 4.x labels Feb 13, 2020
@lychyi lychyi moved this from Backlog to Work (Next Sprint) in Current Sprint (7/20 - 8/9) Feb 18, 2020
@lychyi lychyi moved this from Work (Next Sprint) to Work (This Sprint) in Current Sprint (7/20 - 8/9) Feb 18, 2020
@anicholls anicholls moved this from Work (This Sprint) to In progress (PRs) in Current Sprint (7/20 - 8/9) Feb 24, 2020
@lychyi lychyi moved this from In progress (PRs) to Work (This Sprint) in Current Sprint (7/20 - 8/9) Mar 3, 2020
@jpante jpante moved this from Work (This Sprint) to In Progress in Current Sprint (7/20 - 8/9) Mar 3, 2020
@anicholls
Copy link
Contributor Author

closed by #471

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4.x enhancement New feature or request
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

2 participants