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(button): Simplify Button components and prep for theming #471

Merged
merged 53 commits into from Mar 10, 2020

Conversation

anicholls
Copy link
Contributor

@anicholls anicholls commented Feb 14, 2020

Summary

Refactor our Button components to simplify logic and prep for theming.

Closes #468
Blocked on #481

Todo:

  • IconButton
  • Story file
  • Audit spacing. There are currently inconsistencies with the design specs
  • Focus Rings
  • Filled styling for toggled icon buttons
  • Change target to 4.x
  • Tests
  • Move from labs to replace existing button module
  • Documentation
  • Pull allCaps into a prop for TextButton
  • Factor out IconButtonToggleGroup (refactor: Rename and move IconButtonToggleGroup to SegmentedControl #505)
  • Cypress testing (separate PR)
  • Theming (separate PR)
  • Update changed story URL references in design site

Breaking changes:

  • Some of the button variants have been split into different components to prevent invalid API combinations. DeleteButton, and HighlightButton are now separate components with their own interface. Here are some of the invalid prop combinations that are no longer possible:
    • Delete button with a data label or icon
    • Dropdown button with a data label or icon
    • Highlight button with a data label
    • Highlight button without an icon
    • Dropdown with variants other than primary and secondary
    • Small buttons with an icon or data label
    • Small Highlight button
    • Small Dropdown button
    • etc.
      Required changes:
    • <Button variant={Button.Variant.Delete}> > <DeleteButton>
    • <Button variant={Button.Variant.Highlight}> > <HighlightButton>
    • <Button variant={Button.Variant.OutlineSecondary}> > <OutlineButton>
    • <Button variant={Button.Variant.OutlinePrimary}> > <OutlineButton variant={OutlineButton.Variant.Primary}>
    • <Button variant={Button.Variant.OutlineInverse}> > <OutlineButton variant={OutlineButton.Variant.Inverse}>
  • The majority of the button variant types have changed. BaseButtonProps is no longer available as each button variant has their own interface.
  • React >= 16.8 required for hooks
  • Spacing within buttons has been corrected to match the specs. This may cause horizontal flow changes
  • TextButton now only allows TextButtonSize.Small and TextButtonSize.Medium.
    Required changes:
    • TextButtonSize.Medium > "medium" or TextButtonSize.Small
    • TextButtonSize.Large > "small" or TextButtonSize.Medium
  • All caps variants for TextButton have been turned into a prop
    Required changes:
    • <TextButton variant={TextButton.Variant.AllCaps}> > <TextButton allCaps={true}>
    • <TextButton variant={TextButton.Variant.InverseAllCaps}> > <TextButton variant={TextButton.Variant.Inverse} allCaps={true}>
  • All caps large text buttons now correctly 16px (up from 14)

Quality of Life changes:

With the new components for variants and the simpler types for sizes, the code for complex buttons is much more concise.

Before:

<Button variant={Button.Variant.OutlineSecondary} size={Button.Size.Large}>Label</Button>

After:

<OutlineButton size="large">Label</OutlineButton>

Open questions:

  • Previously we did not allow icons or data labels in small buttons. Are we sure we want to keep this restriction when we've been asked to better support tighter UIs? Note: Small text buttons have always allowed icons
  • Should there be a small dropdown button?

Explanation

Alright - here goes.

First of all, I decided to split up some of the buttons into their own components to avoid invalid API combinations. For example, delete buttons shouldn't allow icons or data labels, but it was still technically possible to do with the old API. We have quite a few cases like this with the old API because we're trying to use the same base interface for everything. Each component now has its own interface specific to its API. We now have Button, DeleteButton, DropdownButton, HighlightButton, TextButton, and deprecated_Button. Note: We will need discussions with design on how to align these with their IA model.

In the old module, the spread of styling functions/data across many files made it really hard to follow what styles were coming from where. Rather than simply duplicating all of that logic for each component, I wanted a way that could keep the CSS for the basic button shape shared, while improving the clarity of what's happening. This led me to strip out ALL of the extra functions, generic styles objects, etc. and rely on React composition + the existing color objects we were already using.

What this means in reality is we now have a ButtonContainer element that accepts a color object of type ButtonColors, which houses all the underlying shared CSS. Each exported button component makes use of this container to share the styling logic. I updated the button color object for more clarity. It now looks like this. Since the ButtonContainer just accepts a color object, this should make it really easy to theme buttons.

In terms of the React composition that I mentioned, gone is the ButtonBase file with all of the label, icon, and data components. The new file structure is very clear, and has one component per file. Other than the shared CSS in ButtonContainer, each component/file houses all of the styling/coloring that it needs. After removing all of the functions, style object files, etc. the file structure is very easy to understand at a quick glance:
image

The deprecated_Button is also completely isolated from the new button styles 🎉 .

Finally, I also refactored some of the styling to remove redundant styles and pull all of the spacing, size and font styling logic out of the labels and into the button container itself.

Any questions or feedback are welcome!

@cypress
Copy link

cypress bot commented Feb 14, 2020



Test summary

155 0 0 0


Run details

Project canvas-kit
Status Passed
Commit 1b96cb9
Started Feb 20, 2020 11:30 PM
Ended Feb 20, 2020 11:34 PM
Duration 03:42 💡
OS Linux Ubuntu Linux - 16.04
Browser Electron 78

View run in Cypress Dashboard ➡️


This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

@NicholasBoll
Copy link
Member

What about adding a HyperlinkButton? We've been asked to do this the use-case of file downloads: LRN-15985

I did this with a bit of hacking: https://codesandbox.io/s/hyperlinkbutton-jh7re

Basically the ask is an anchor tag with the styling of a button.

@anicholls
Copy link
Contributor Author

@NicholasBoll Yeah we already have an issue open for this: #87. I definitely think it would be good to add, but not sure it should happen in this PR.

@lychyi lychyi added this to In progress (PRs) in Current Sprint (7/20 - 8/9) via automation Feb 18, 2020
@anicholls anicholls added 4.x breaking-change A change that will break something for consumers labels Feb 20, 2020
@anicholls
Copy link
Contributor Author

@lychyi Re #378 - Now that we've removed our dependency on babel-plugin-emotion, we no longer need to build buttons with Babel AFAIK. We should be able to use the TS compiler like the rest of our modules.

@anicholls anicholls changed the base branch from master to prerelease/v4 February 20, 2020 23:06
@anicholls anicholls changed the title DNM - refactor: Button changes refactor(button): Simplify Button components and prep for theming Feb 20, 2020
@anicholls anicholls marked this pull request as ready for review February 20, 2020 23:20
@cypress
Copy link

cypress bot commented Feb 21, 2020



Test summary

185 0 0 0


Run details

Project canvas-kit
Status Passed
Commit 9985be7 ℹ️
Started Mar 10, 2020 5:32 PM
Ended Mar 10, 2020 5:35 PM
Duration 02:59 💡
OS Linux Ubuntu Linux - 16.04
Browser Electron 78

View run in Cypress Dashboard ➡️


This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

@lychyi
Copy link
Contributor

lychyi commented Feb 24, 2020

@anicholls Yup, for now I don't think we need Babel for button transpilation. It may require removing some of the files made just for that build process so the change may not be that straight forward.

If we want to add Emotion 11 into v4, then we can once again use component selectors with the babel plugin. In addition, Babel provides us an opportunity to transpile using other plugins so we may end up going that route anyway.

Personally, I don't mind leaving it. But if we want a clean slate and switch to tsc, I'll leave that up to you in terms of weighing the refactoring of the build vs. just leaving it and updating Babel.

@anicholls anicholls mentioned this pull request Feb 24, 2020
12 tasks
@anicholls anicholls force-pushed the button-refactor branch 2 times, most recently from d468dee to 1b78e9d Compare February 26, 2020 18:12
Copy link
Contributor

@lychyi lychyi left a comment

Choose a reason for hiding this comment

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

I think this is a great refactor. I'm deferring to the button experts though: @mannycarrera4 ?

@anicholls anicholls merged commit df817d8 into Workday:prerelease/v4 Mar 10, 2020
@anicholls anicholls deleted the button-refactor branch March 10, 2020 17:48
@anicholls anicholls added this to the v4.0.0 milestone Mar 31, 2020
@anicholls anicholls mentioned this pull request Jun 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4.x breaking-change A change that will break something for consumers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor React Button
4 participants