UX-436 Migrate Button 🐐 #39

mikrotron · 2019-02-22T01:58:16Z

🛠 Purpose

Add a <Button> component that can be rendered as either a <button> or global CSS resilient <RawButton>.

✏️ Notes to Reviewer

Buttons with icons are not yet supported since an <Icon> component has not yet been added.
Added a very basic Storybook story for now.
Updated Tokens package with latest design tokens / script fix.
Added customPropTypes helpers, including a ShirtSizes enum.
No tests (yet).
Resolves Button Component #40

🖥 Screenshots

KirillKayumov · 2019-02-22T10:14:26Z

.storybook/assets/styles/common.styles.js

@@ -14,3 +15,9 @@ export const CenteredStory = styled.div`
  justify-content: center;
  width: 100%;
 `;
+
+export const Rule = styled.hr`
+  border: 1px solid ${tokens.border.color};


Here you define 4 borders with 1px width just to define color. Then below you remove all borders except one. And you need to think a little bit to understand what border we eventually have.
Can we define it like this?

border-bottom: 1px solid ${tokens.border.color};

Because this is applied to an <hr> element, which already gets four borders from the user agent styles, I need to also remove the other borders for a 1px line effect. An alternative would be to set the border to none, then define border-bottom, as you suggest, or maybe in one, pretty long line.

alternative would be to set the border to none, then define border-bottom

👍

FYI: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/hr

The HTML <hr> element represents a thematic break between paragraph-level elements: for example, a change of scene in a story, or a shift of topic within a section.
Historically, this has been presented as a horizontal rule or line. While it may still be displayed as a horizontal rule in visual browsers, this element is now defined in semantic terms, rather than presentational terms, so if you wish to draw a horizontal line, you should do so using appropriate CSS.

I think this means that if we just want a horizontal line, we should not be using <hr> (even though it is indeed separating<p>s in the stories. Those paragraphs are not really textual, so there can't be a "a change of scene in a story, or a shift of topic within a section". Yes, I know it's just for stories.)

But this is a story. The story of a component. And especially for stories written for Screener, I think a "shift of topic within a section" is basically what we'd want this component for. A break between the description / illustration of a component's different props. At least, this is one way to think of it that doesn't violate the semantics of the <hr>.

I'm not crazy about the name <Rule>, but wasn't too concerned about it for this pr. But maybe a better name would be something like <SectionBreak> or <TopicShift> or something like that. At any rate, we'll need to give this some careful though when we get to setting up the Storybook in a clearer, more consistent way.

KirillKayumov · 2019-02-22T10:28:30Z

packages/Button/Button.styles.js

+  ${props.isFullWidth ? fullWidthStyles : ""}
+  ${props.isActive ? activeStyles : ""}
+  ${props.isDisabled ? disabledStyles : ""}
+  ${["minor", "link"].includes(props.type) && props.isDisabled ? disabledTextStyles : ""}


Since the disabledTextStyles are relevant only for these two types, i think that it should be conditionally inserted inside typeStyles for minor and links types.

const typeStyles = (props) => ({ ..., minor: ` ${textButtonStyles} ${props.isDisabled && disabledTextStyles} &:hover { text-decoration: underline; } `, ... }[props.type]); const composedStyles = props => ` ... ${typeStyles(props)} ... `;

KirillKayumov · 2019-02-22T10:30:20Z

packages/Tokens/tokens.yaml

@@ -9,19 +9,22 @@ color:
  black-lighten-70: desaturate(lighten($color--black, 70), 5)
  black-lighten-80: '#f7f7f7'
  black-lighten-90: '#fcfcfc'
+  black-disabled: $color--black-lighten-40


Just curious: what's the reason of using .yaml extension instead of .yml?

there is no reason at all, I think this is the extension assigned when the script was created. We can totally rename if we want is a simple character https://github.com/acl-services/paprika/blob/master/packages/Tokens/script.js#L7

KirillKayumov · 2019-02-22T11:25:00Z

packages/helpers/customPropTypes.js

+
+const sizes = ["xsmall", "small", "medium", "large", "xlarge"];
+
+export const shirtSize = (props, propName, component) => {


Can we reuse PropTypes.oneOf here?

import PropTypes from 'prop-types'; const sizes = ["xsmall", "small", "medium", "large", "xlarge"]; export const shirtSize = PropTypes.oneOf(sizes);

An idea wouldn't be good to have an Enum definition for our library, something like enumType.xsmall, enumType.small, etc.

so we could do.

PropTypes.oneOf([EnumType.xsmall, EnumType.small]) this way is more descriptive when the developer reads it, and as well we can reuse the Enum across the library?

I'd say if we do use some form of enum, its ergonomics should be good enough that we don't have to list all the possible options explicitly inside oneOf(...)

So if I define the enum as:

ShirtSize = { SMALL: 'small', MEDIUM: 'medium', LARGE: 'large', }

Then I could also add a line like this:

ShirtSize.ALL = Object.values(ShirtSize);

So that we could use it like:

PropTypes.oneOf(ShirtSize.ALL);

Does that seem like a good pattern?
Do you think ANY would be better than ALL? Do you think ShirtSizes would read better than ShirtSize? Do we really need an Enum prefix/postfix?

KirillKayumov · 2019-02-22T11:45:49Z

packages/Button/Button.js

+  }
+
+  setRef(props) {
+    this.$button = props.buttonRef || React.createRef();


Could you please clarify when we indeed need a reference to a Button? I see that it's used now in the Storybook to print info about clicked button but you can get all required data from event object inside onClick callback.

I believe that everything should be done on purpose, so if there is currently no case when we need this, I'd suggest getting rid of it.

Also, is it a good practice to call React.createRef() on every button re-render if buttonRef is not passed through props?

This should work.

const Button = React.forwardRef((props, ref) => { const handleClick = event => { if (!props.canPropagate) event.stopPropagation(); if (!props.isDisabled) props.onClick(event); }; const { ariaText, buttonRef, canPropagate, children, isDisabled, isSemantic, tabIndex, ...moreProps } = props; if (ariaText) moreProps["aria-label"] = ariaText; const buttonProps = { isDisabled, onClick: handleClick, ref, tabIndex, ...moreProps, }; if (isSemantic) { buttonProps.disabled = isDisabled; } else { buttonProps.tabIndex = isDisabled ? -1 : tabIndex; } const ButtonTag = isSemantic ? ButtonStyled : RawButtonStyled; return <ButtonTag {...buttonProps}>{children}</ButtonTag>; });

and using it:

function MyComponent() { const $ref = $ref.createRef() return <Button ref={$ref} /> }

I think we need to address this for the <RawButton> first, so I've opened a separate PR for that purpose: #42

KirillKayumov · 2019-02-22T11:46:21Z

packages/Button/Button.js

+
+    const ButtonTag = isSemantic ? ButtonStyled : RawButtonStyled;
+
+    return <ButtonTag {...buttonProps}>{children}</ButtonTag>;


children can be left in moreProps.

Seems odd to write {moreProps.children} here, and a bit cleaner the way it is (vs this.props.children).

KirillKayumov · 2019-02-22T11:48:46Z

packages/Button/Button.js

+
+  render() {
+    const { ariaText, buttonRef, canPropagate, children, isDisabled, isSemantic, tabIndex, ...moreProps } = this.props;
+    if (ariaText) moreProps["aria-label"] = ariaText;


Since ariaText is used only to set value for aria-label, why do we need this prop?

^ also, couldn't we detect if the children are typeof String and in that case just assigned it to the aria-text otherwise, if includes a React Node, We could trigger a warning asking the developer to add a meaningful aria-text?

We could even transform this into a propType validation?

@nahumzs, I don't think that it's a good idea to assign aria-label automatically. If children is String then it's not really necessary to specify aria-label, the button already has text, so e.g. VoiceOver can pronounce what the button is. The same as children is a text + icon.
But if children is only an icon (no text at all) then it'd be great to force a consumer to specify aria-label.

You're right, @KirillKayumov, we don't really need an explicit prop, we can just rely on the ...moreProps mechanism to forward aria-label on to the <ButtonTag>.

However, I have a suggestion that to insulate app developers from the implementation details of the a11y of our components, we could consistently use a "proprietary" ariaText prop for all of our Paprika components, and that could be used for the component in the best way. Sometimes that's simply an aria-label on the root node, but in some cases it might be on a specific element that is not the root node, or use a different attribute, like aria-labelledby, or even a different technique, like a <caption> or <legend>. Do you think this kind of API actually makes sense and makes the app developer's experience more delightful?

After a bit of discussion with Nahum, we decided to stray even further in the naming to avoid confusion, and call this prop a11yText. The goal is to reduce the work (cognitive load) for the app developers.

KirillKayumov · 2019-02-22T12:13:55Z

packages/Button/Button.js

+  buttonRef: PropTypes.shape({ current: PropTypes.instanceOf(Element) }),
+  canPropagate: PropTypes.bool,
+  children: PropTypes.node.isRequired,
+  className: PropTypes.string,


Do we really need to specify className here?

Nope, ✂️

KirillKayumov · 2019-02-22T12:17:22Z

packages/Button/stories/examples/Basic.js

+        raw small link
+      </Button>
+    </p>
+    {[


I think it's worth to render a "not semantic" button in this story as well.

Not semantic buttons are <RawButtons> (or whatever we decide to rename them to, that is still open for debate 🙂), and the three examples above here are, as specified by isSemantic={false}.

This component's stories will be revisited though in a subsequent task/PR that is concerned specifically with Storybook design patterns.

KirillKayumov · 2019-02-22T12:27:56Z

packages/Button/Button.js

+      ...moreProps,
+    };
+
+    if (isSemantic) {


I think this condition is redundant because you can safely specify both disabled and tabIndex props for both ButtonStyled and RawButtonStyled components. Can we re-write the render function like so?

buttonProps() { const { isDisabled, tabIndex } = this.props; return { ...this.props, disabled: isDisabled, onClick: this.handleClick, ref: this.$button, tabIndex: (isDisabled ? -1 : tabIndex), }; } render() { const ButtonTag = this.props.isSemantic ? ButtonStyled : RawButtonStyled; return <ButtonTag {...this.buttonProps()} />; }

This is simpler, and I'm tempted to take this suggestion, despite the concerns I have that made me choose this more convoluted approach:

disabled is going to end up passed along by a "raw" <Button> to the underlying <span> and it will be invalid (may throw warnings, I'll try and see).

The tabIndex logic will be duplicated in a "raw" <Button>, which may not be a big deal, it's just a bit unnecessary.

Add to that:
3. type (submit or button) is being added to isSemantic buttons, but not <RawButtons>
And I think I have teetered back over to the other side again and prefer the branching logic.

nahumzs · 2019-02-22T21:06:05Z

packages/Button/Button.js

+    if (ariaText) moreProps["aria-label"] = ariaText;
+
+    const buttonProps = {
+      isDisabled,


you should add type="button" to avoid the default value "submit" https://github.com/yannickcr/eslint-plugin-react/blob/master/docs/rules/button-has-type.md

What if we do want a submit button? I'm thinking that our type prop clashes with the HTML type attribute, so there's no way to make a submit button...

so what we need is a default prop be type='button', but the type is need it if not all button will be submit buttons.

I think what we need is two separate props like type (= submit | button | reset) and... kind? (= primary | link | ...)

^ yeah that makes sense.

While I mostly agree, two points:

I don't believe in <button type='reset'> so really the only options are submit | button. Most of the time we want button. So we could utilize a boolean prop, like isSubmit which is potentially a bit simpler.

The above would allow us to keep the exising API of using type as the prop for this component... but I don't like that either, since type is a valid attribute in this case and it makes it a bit confusing... so I still like the idea of using a different name, like kind. Or theme? Or, taking inspiration from BluePrintJS, intent?

Aside: I don't think we should ever use props that are valid attributes in our new props APIs. title, for example, should be avoided.

👍 on isSubmit
👍 on kind, 😐 on intent (seems like their notion is more narrow than what we need, e.g. doesn't include things like minor or link), 👎 on theme since it's a loaded word in the component library world

Aside: I don't think we should ever use props that are valid attributes in our new props APIs. title, for example, should be avoided.

To clarify, does that mean we don't want for there to be an intersection between our prop names and DOM attribute names? seems like too strict and sounds like you would e.g. prefer to use type prop for the type attribute?

To move forward, how about I go with kind?
But, like the name of the, RawButton, we should revisit and get a bit wider feedback on this before we decide definitively (before we release).

Also, I think if the prop is controlling the same attribute, we can keep the same name, as you suggest at the end of your last comment. But when it would refer to two unrelated things, like type would for this component, let's avoid it.

alexzherdev · 2019-02-25T22:04:40Z

.storybook/assets/styles/common.styles.js

@@ -14,3 +15,9 @@ export const CenteredStory = styled.div`
  justify-content: center;
  width: 100%;
 `;
+
+export const Rule = styled.hr`
+  border: 1px solid ${tokens.border.color};


FYI: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/hr

The HTML <hr> element represents a thematic break between paragraph-level elements: for example, a change of scene in a story, or a shift of topic within a section.
Historically, this has been presented as a horizontal rule or line. While it may still be displayed as a horizontal rule in visual browsers, this element is now defined in semantic terms, rather than presentational terms, so if you wish to draw a horizontal line, you should do so using appropriate CSS.

I think this means that if we just want a horizontal line, we should not be using <hr> (even though it is indeed separating<p>s in the stories. Those paragraphs are not really textual, so there can't be a "a change of scene in a story, or a shift of topic within a section". Yes, I know it's just for stories.)

alexzherdev · 2019-02-25T23:58:29Z

packages/helpers/customPropTypes.js

+
+const sizes = ["xsmall", "small", "medium", "large", "xlarge"];
+
+export const shirtSize = (props, propName, component) => {


I'd say if we do use some form of enum, its ergonomics should be good enough that we don't have to list all the possible options explicitly inside oneOf(...)

alexzherdev · 2019-02-26T00:05:56Z

packages/Tokens/script.js

@@ -121,7 +121,7 @@ import TokenSquare from "./TokenSquare"
    const matches = str.match(/\$[\w'-]+/gi);
    if (matches) {
      matches.forEach(match => {
-        const re = new RegExp(match.replace("$", "\\$"), "gi");
+        const re = new RegExp(match.replace("$", "\\$"), "i");


Why not g?

Because:
https://github.com/acl-services/acl-ui/pull/949/commits/f39e9d373e4ba57e52c0fd9cfaa7275b7c690fef#r211027057
So:
https://github.com/acl-services/acl-ui/pull/949/commits/c4426a347ec45615d11e407c7a3f5fe95d5ee906

alexzherdev · 2019-02-26T00:10:22Z

packages/Button/package.json

+    "prop-types": "^15.7.2"
+  },
+  "peerDependencies": {
+    "react": "^16.4.0",


Not 16.8.4?
https://www.npmjs.com/package/react

alexzherdev · 2019-02-26T00:33:11Z

packages/Button/Button.js

+
+const propTypes = {
+  ariaText: PropTypes.string,
+  buttonRef: PropTypes.shape({ current: PropTypes.instanceOf(Element) }),


N.B.: facebook/prop-types#240

alexzherdev · 2019-02-26T00:38:50Z

packages/Button/Button.styles.js

+  }
+`;
+
+const commonPostStyles = `


What does post mean here?

They needed to come later, to override some styles above it... but it seems to no longer be necessary.

alexzherdev · 2019-02-26T00:40:34Z

packages/Button/Button.styles.js

+
+// Modifiers
+
+const fullWidthStyles = `


Hmm it feels a bit weird to have a dedicated full width variant. Can we leave it up to the consumer to style?

In general, fewer responsibilities for the consumer, the better. Especially CSS responsibilities... that way the rules of the design system can be enforced. And it makes the experience simpler for the app developer (in theory).

But we don't know what "full width" means in the consumer's context. Sometimes it's width: 100%, other times it's flex-grow: 1, and maybe even other times it could be display: block? I think this is starting to be layout territory where IMO we should give flexibility. I've definitely heard advice like making sure there's a way to use a component both inline and as a block (if it makes sense), I think it's a similar concern.

alexzherdev · 2019-02-26T00:42:08Z

packages/Button/Button.styles.js

+`;
+
+export const ButtonStyled = styled.button`
+  ${props => composedStyles(props)}


Normally I would prefer inlining but in styled components I guess it's more natural to see the props argument?

Nope, this is a good suggestion, and I like it better as simply ${composedStyles}.

alexzherdev · 2019-02-26T00:42:41Z

packages/Button/package.json

+{
+  "name": "@paprika/button",
+  "version": "0.0.1",
+  "description": "Button component",


This will be the package description published to npm. We should probably start using something more meaningful.

Good point, but how descriptive do we need to be for a button? "Skeuomorphic, accessible button component" ?

We intend to address this for all of our components before a 1.0.0 release and hopefully we can recruit a proper wordsmith the help with it.

alexzherdev · 2019-02-26T00:43:41Z

packages/Button/Button.styles.js

+  }
+
+  &:active {
+    box-shadow: ${tokens.highlight.active.noBorder.boxShadow}, inset 0 1px 1px 0 rgba(0,0,0, 0.1),


rgba(0,0,0, 0.1) is this irregular spacing on purpose?

No, I'm surprised it wasn't fixed by prettier, but despite the fact I think it's actually easier to read, I don't think it should be irregular, as you put it. Will change.

nahumzs

Let's 🚢 this one, and address any other comment in another PR.

mikrotron changed the title ~~UX-436 Migrate utton~~ UX-436 Migrate Button Component 🐐 Feb 22, 2019

mikrotron changed the title ~~UX-436 Migrate Button Component 🐐~~ UX-436 Migrate Button 🐐 Feb 22, 2019

mikrotron added WIP 📦 New Package A new shiny Component labels Feb 22, 2019

KirillKayumov reviewed Feb 22, 2019

View reviewed changes

nahumzs reviewed Feb 22, 2019

View reviewed changes

alexzherdev reviewed Feb 26, 2019

View reviewed changes

mikrotron removed the WIP label Mar 7, 2019

Mike Polowick added 11 commits March 14, 2019 14:58

Token updates; Add customPropTypes helper;

339c1ac

Setup Button component package + basic storybook story

c821d1f

Implement isSemantic prop; Update and improve storybook story;

d6d246a

Compose disabled styles better; Adjust CSS for Rule;

5b4e4b1

Code cleanup as suggested in code review

2fda248

Enum ShirtSizes

8704ba2

Prop rename: type -> kind; Add isSubmit prop;

376f0de

Add hasInsetFocusStyle to RawButton

d9c6c81

ariaText -> a11yText

3741b57

Use React.ForwardRef, not buttonRef prop; Add stories;

851f148

Clean up storybook timers

ab031b6

mikrotron force-pushed the UX-436/migrate-button branch from fc687b0 to ab031b6 Compare March 14, 2019 23:01

nahumzs approved these changes Mar 14, 2019

View reviewed changes

mikrotron merged commit 418d4d8 into master Mar 14, 2019

mikrotron deleted the UX-436/migrate-button branch March 14, 2019 23:39


		const sizes = ["xsmall", "small", "medium", "large", "xlarge"];

		export const shirtSize = (props, propName, component) => {


		const ButtonTag = isSemantic ? ButtonStyled : RawButtonStyled;

		return <ButtonTag {...buttonProps}>{children}</ButtonTag>;

UX-436 Migrate Button 🐐 #39

UX-436 Migrate Button 🐐 #39

Conversation

mikrotron commented Feb 22, 2019 • edited Loading

🛠 Purpose

✏️ Notes to Reviewer

🖥 Screenshots

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexzherdev Mar 6, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nahumzs left a comment

Choose a reason for hiding this comment

mikrotron commented Feb 22, 2019 •

edited

Loading

alexzherdev Mar 6, 2019 •

edited

Loading