[PAY-1651] Implements Harmony Buttons by schottra · Pull Request #3794 · AudiusProject/audius-client

schottra · 2023-07-25T15:35:17Z

Description

This adds the initial implementation of Harmony buttons, based on the new design system. Notable updates:

Cleaned up size and color variant definitions. We now mostly just update some CSS variable values as the color variant class changes.
Using gap for layout now, so no special classes for icons/text based on presence of each other
Stories render disabled variants now
Icon properties accept an icon component instead of a rendered element, allowing the button to control when the icon is actually rendered and apply classes directly to the SVG instead of a span above it.
SVG paths will now use currentColor for the fill, so that they always match the text color
Added a deprecation jsdoc comment to the original Button to remind devs to use the new component in new feature work.
Renamed iconCampFire.svg -> iconCampfire.svg to match Figma
Remove some unnecessary semantic color definitions which don't differ across themes (the variables they reference change)
Added semantic colors for border/background since they are used in the new button

Dragons

No changes to existing button component and this component is currently unused, so it should be a safe change.

How Has This Been Tested?

Locally verified in Storybook

How will this change be monitored?

N/A

Feature Flags

N/A

Screenshots

primary-button-click.mp4

other-button-clicks.mp4

audius-infra · 2023-07-25T16:22:14Z

Preview this change https://demo.audius.co/pay-1651-harmony-buttons

dylanjeffers

wonderful work, so simple and clean

dylanjeffers · 2023-07-25T16:39:45Z

  --text-disabled: var(--neutral-light-7);

+  /* Semantic Borders */
+  --border-default: var(--neutral-light-8);


dylanjeffers · 2023-07-25T16:40:34Z

+}
+
+.button:focus {
+  outline: none !important;


do we want to do a css reset on button? mabye all: unset on the root element`

I had to look up how this works

Specifies that all the element's properties should be changed to their inherited values if they inherit by default, or to their initial values if not.

I'm actually a bit confused on how it's useful. Could you give me an example scenario you're trying to account for?

dylanjeffers · 2023-07-25T16:44:40Z

+  padding: var(--unit-5) var(--unit-6);
+}
+
+.large .icon {


i think this is totally good, but wondering how you feel about specificity changes here? ive personally always tried to just have classname directly tied to the element, so i might do largeIcon class. Im no expert though so would love your insight here

and for example if we pass in iconClassName, doesnt specificity get a bit messed up when having to compete against .default .icon?

Yeah good call. I was actually trying to use the least specificity possible, but I didn't really want to do separate classes for each piece (button, icon, text). I used a handy specificity calculator to check, and you are correct! The only way to make sure the class can be overridden is to use only one class, no descendants or combinators. Will update.

dylanjeffers · 2023-07-25T16:45:18Z

+
+/* Secondary */
+.secondary {
+  --button-color: var(--neutral-light-5);


really cool with the css-vars defined in the className. do you mind giving a quick tldr on why we do this again?

Yep! There were two goals:

Allowing override of the button color from JS by injecting a different value for the variable. (This is only used in primary buttons at the moment)

Explicitly linking various properties logically to the button color. The idea is that you think about the button having a color, and then the style rules for each variant decide how the color is applied to the button (ex. Secondary buttons apply the color to only the border, primary apply the color to the background).

Defining how the color maps to the properties once, then just changing the color for hover/active states.

But I could see how this is arguably not any easier to follow than just setting the individual values per variant, per state. Open to simplifying it if you think it reads better.

thank you for all the context, this makes sense, esp with allowing it styled through js. lets continue with this pattern and see how folks get along with it 🔥

dylanjeffers · 2023-07-25T16:46:29Z

+ * include and position icons.
+ */
+export const HarmonyButton = forwardRef<HTMLButtonElement, HarmonyButtonProps>(
+  function Button(


nice! maybe also call this HarmonyButton?

dylanjeffers · 2023-07-25T16:47:47Z

+ */
+export const HarmonyButton = forwardRef<HTMLButtonElement, HarmonyButtonProps>(
+  function Button(
+    {


just since this will be our "core components" ill be extra nitty just to make sure we have alignment. so i personally prefer function Component(props) {...} and then destructure props in the body. i find this makes the top-level function definition a but easier to parse. if you feel strongly this way, all good as well, just curious :)

i forget but i think it also may prevent an extra nested body definition.

I actually don't find either way more or less readable. Happy to destructure in the function body. I thought there was a style guide entry about doing it in the arguments, but I was unable to find it.

yeah its not official, just something i personally like and wanted to start a convo :)

dylanjeffers · 2023-07-25T16:48:39Z

+      widthToHideText,
+      minWidth,
+      className,
+      iconClassName,


so as a pattern going forward do we want nested classnames to be indivudally named? in mobile and other libraries ive seen the classes pattern where it's an object of all the inner classnames we would want to open up.

Ideally, we shouldn't have class overrides at all :-)
But I'm fine with either pattern, as long as it's consistent. I think the classes one feels a little more generic and maps more directly to how we do styling in modules/JS (i.e. styles.icon, styles.text). This probably deserves broader discussion with other folks who will use it.

For now, I am not planning on using any icon/text class overrides in new features. If you want, I can remove it for now so it's not possible, until we make a formal decision?

I will say that with the classes pattern, it might be harder to write types such that you know what the valid class names are.
For example,

// Harder to write a props shape that will catch this typo! <HarmonyButton classes={icn: styles.icon, text: styles.text} />

Not impossible, I could whip up a generic that makes it easier to define. Just wanted to point that out.

yah totally agreed not having to pass styles in, so just removing it is great for now, and then we can all have a discussion about what pattern we want moving forward!! as far as classes type utility we added something somewhat basic in mobile that works well, so can prob reuse that if we go forward with classes

dylanjeffers · 2023-07-25T16:51:18Z

+        )}
+        disabled={disabled}
+        ref={ref}
+        style={style as CSSProperties}


just wondering about CSSCustomProperties vs CSSProperties. anyway to avoid the cast?

Oh right, I did work around this in a cleaner way elsewhere by just casting an object directly to CSSProperties.
The issue is that the built-in CSSProperties doesn't allow you to pass custom variables (--button-color)

dylanjeffers · 2023-07-25T16:52:48Z

+      text,
+      variant = HarmonyButtonType.PRIMARY,
+      size = HarmonyButtonSize.DEFAULT,
+      leftIcon: LeftIconComponent,


headsup mobile does iconLeft. im fine with either, but prob want consistency. if we go with leftIcon, can we make a note in mobile code on the prop definitions that we'd like to rename?

Oh I'd rather this be consistent. Can switch it to iconLeft. I think it actually works better because then iconLeft and iconRight sort next to each other lexographically.

sliptype

Really awesome work! I think yall already covered everything I was going to point out. This is such a solid start to Harmony 🎉

audius-infra · 2023-07-25T18:13:31Z

Preview this change https://demo.audius.co/pay-1651-harmony-buttons

[5e99303] Add favorite test and fix aria-label (#3817) Raymond Jacobson [ccc32ce] [C-2908 C-2744] fix desktop follow button (#3816) Dylan Jeffers [c0679c3] [PAY-1660] Fix layout issues with TrackTile socials row with a lot of stats (#3815) Randy Schott [089a9e6] Pin stripe package versions (#3813) Reed [a281267] [C-2774] Update upload inputs (#3806) Dylan Jeffers [f504ef9] [C-2901] Fix menu types (#3811) Dylan Jeffers [cb9a385] [C-2905] Update Text types and props to camelCase (#3810) Kyle Shanks [027b3a5] [PAY-1624] Implement Purchase modal (#3808) Randy Schott [deadb5f] [C-2902] Update the upload forms to use the typography component (#3809) Kyle Shanks [039c951] [C-801] Fix oauth nodes (#3807) Raymond Jacobson [c3765c7] Update typography component to use classnames (#3805) Kyle Shanks [cab0a3e] Switch to Stripe package instead of script (#3798) Reed [a84126f] [C-2890] Add first version of a typography component to web (#3796) Kyle Shanks [4addddc] Fix mobile prem-content drawer unlocking margin (#3804) Reed [233b585] [C-2857] Remove get blocknumber (#3802) Dylan Jeffers [d113bdb] Prepare for 1.5.34 full app release (#3801) Dylan Jeffers [2f09db4] [C-2887] Fix collection button widths (#3800) Dylan Jeffers [8158e10] [PAY-1655] Add ColorValue prop to Text component (#3799) Reed [fe4bc6a] Revert cacheActions.add thunk (#3797) Dylan Jeffers [2370bbe] [PAY-1650] Update play/preview buttons on track details to use HarmonyButton (#3795) Randy Schott [3579dc2] [PAY-1651] Implements Harmony Buttons (#3794) Randy Schott [5af77ec] [C-2886] Improve cache performance (#3792) Dylan Jeffers [6fb78f1] [PAY-1587] Mobile USDC Purchase Drawer Skeleton (#3793) Reed [1277a41] [C-2883] Migrate confirmer to common (#3788) Dylan Jeffers [ce2548e] [plat-1111] add usdc purchase seller and buyer notifications (#3770) sabrina-kiam [bc04f52] Fix mobile LockedStatusBadge padding (#3790) Reed [8943078] [C-2680] Attribution Modal (#3778) Andrew Mendelsohn

schottra added 6 commits July 24, 2023 12:11

initial file structure for harmony button

2f69b51

add deprecation for old Button class

cec542a

checkpoint

13af95a

finishing up initial styling pass for harmony buttons

24c8eb2

final fixes for harmony buttons

6a9782d

cleanup

f23bb14

pull-request-size Bot added the size/XL label Jul 25, 2023

schottra requested review from dylanjeffers and sliptype July 25, 2023 15:35

schottra added 3 commits July 25, 2023 11:39

more cleanup

9656ab1

more cleanup again

a5d1257

correctly rename campfire icon

13b9a1f

schottra assigned rickyrombo and piazzatron Jul 25, 2023

update type for icons

0b73c19

dylanjeffers approved these changes Jul 25, 2023

View reviewed changes

PR feedback

f205e9a

sliptype approved these changes Jul 25, 2023

View reviewed changes

schottra merged commit 3579dc2 into main Jul 25, 2023

schottra deleted the pay-1651-harmony-buttons branch July 25, 2023 19:45

AudiusProject deleted a comment from linear Bot Sep 11, 2023

Conversation

schottra commented Jul 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Dragons

How Has This Been Tested?

How will this change be monitored?

Feature Flags

Screenshots

Uh oh!

audius-infra commented Jul 25, 2023

Uh oh!

dylanjeffers left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dylanjeffers Jul 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sliptype left a comment

Choose a reason for hiding this comment

Uh oh!

audius-infra commented Jul 25, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

schottra commented Jul 25, 2023 •

edited

Loading

dylanjeffers Jul 25, 2023 •

edited

Loading