[web] Introduce plain Button variant
ClosedPublic
Actions

Authored by michal on Oct 11 2022, 6:40 AM.

Details

Reviewers

tomek
atul
• abosh
ginsu

Commits

rCOMM9bfd7a1c01b9: [web] Introduce plain Button variant

Summary

Part of ENG-843
Introduces plain Button variant with minimal css properties. It becomes the default variant, so this diff also changes any Buttons without explicit variant to use filled.

Also makes the onClick nullable and sets the type to button to make it more general.

This variant is used in a later diff.

Test Plan

Run flow
Check if all changed buttons look and work the same
Check if there is any <form> that uses a Button without an explicit type set

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

michal created this revision.Oct 11 2022, 6:40 AM

Herald added a subscriber: ashoat. · View Herald TranscriptOct 11 2022, 6:40 AM

michal added a parent revision: D5327: [web] Use <a> instead of <div> in some places.Oct 11 2022, 6:41 AM

Harbormaster completed remote builds in B12728: Diff 17489.Oct 11 2022, 6:50 AM

michal requested review of this revision.Oct 11 2022, 6:50 AM

michal edited the summary of this revision. (Show Details)Oct 11 2022, 6:54 AM

michal edited the test plan for this revision. (Show Details)

michal added a child revision: D5343: [web] Use plain Button instead of html elements.Oct 11 2022, 7:17 AM

Looks good, just a suggestion on using different variant of classnames(...).

web/components/button.react.js
38–43 ↗	(On Diff #17489)	There's another variant of `classnames(...)` that we could use here. It would look something like: const btnCls = classnames({ [css.plain]: true, [css[variant]]: true, [css.btn]: variant !== 'plain', [css[buttonColor]]: typeof buttonColor === 'string', }); Personally think it would be a little cleaner/easier to read. If you do decide to go with that approach, I'd make sure to do some testing and make sure that buttons appear as expected.