-
Notifications
You must be signed in to change notification settings - Fork 9
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
UX-433 Migrate RawButton π #32
Conversation
packages/RawButton/RawButton.js
Outdated
@@ -0,0 +1,119 @@ | |||
/* eslint-disable jsx-a11y/no-static-element-interactions */ | |||
/* eslint-disable jsx-a11y/click-events-have-key-events */ | |||
/* es-lint is disabled for these specific reasons: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought it was cleaner to add all of these notes to the top of the file, instead of inline in the code.
packages/RawButton/RawButton.js
Outdated
|
||
const propTypes = { | ||
ariaText: string, | ||
buttonRef: shape({ current: instanceOf(Element) }), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No me gusta. But...
facebook/prop-types#240
packages/RawButton/RawButton.js
Outdated
*/ | ||
|
||
import React from "react"; | ||
import { string, number, bool, func, node, shape, instanceOf } from "prop-types"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was there a reason not to do it this way, and do import PropTypes from "prop-types"
instead, @alexzherdev?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same argument as React.Component
. So I feel like it makes sense to do both the same way.
packages/RawButton/RawButton.js
Outdated
|
||
componentDidMount() { | ||
if (this.$rawButton) { | ||
this.$rawButton.current.addEventListener("keydown", this.handleKeyDown); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have removed the useCapture
parameter we had before. After some more reading, I don't think we should need it here...?
https://www.w3.org/TR/DOM-Level-3-Events/#event-flow
https://www.quirksmode.org/js/events_order.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have to define event listeners using addEventListener
only because of the 1. Click-events-have-key-events
comment above?
I've tried to use normal onKeyDown
and page wasn't scrolled when space key pressed.
<RawButtonStyled
{...moreProps}
onKeyDown={this.handleKeyDown}
/>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Originally I assumed these were needed and just brought them over from the original component. But I'm going to try to use React events instead, sounds like it will work just fine.
packages/RawButton/RawButton.js
Outdated
export default class RawButton extends React.Component { | ||
constructor(props) { | ||
super(props); | ||
this.$rawButton = props.buttonRef || React.createRef(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No idea if this is the right way to forward a ref in this case? It works though π
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So it sounds like there's two schools of thought.
<RawButton ref={someRef} />
where someRef
is forwarded to some inner DOM element with React.forwardRef
. By virtue of being called ref
it's transparent that you're putting a ref on a React component (RawButton
), not an actual DOM element. This assumes no one will ever need to grab a ref for the RawButton
itself.
<RawButton buttonRef={someRef} />
The "old" way. styled-components itself does this with innerRef
.
Thoughts?
NB. if we do props.buttonRef
in the constructor, we also need to handle someone swapping it for a different value in componentDidUpdate
(would be much easier with hooks π)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer 2, since you still have the option of setting a ref
on the <RawButton>
component. It may not be needed for this component, but I like that it's a pattern that can scale. Also, I think there are even some components where you might want a ref
to multiple underlying elements, so naming them not only gives you a convention that works in that case too, but also gives a little more info about what kind of element you're getting a reference for.
}; | ||
|
||
handleClick = event => { | ||
if (!this.props.canPropagate) event.stopPropagation(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keeping the canPropagate
prop for this case, which I think will be useful.
packages/RawButton/RawButton.js
Outdated
<RawButtonStyled | ||
{...moreProps} | ||
aria-disabled={isDisabled} | ||
data-qa-anchor={`paprika-raw-button ${qaAnchor}`} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thoughts about this? π€
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you provide me a use case where this is necessary? Why can't we just use paprika-raw-button
?
I guess one example is a test that has a bunch of RawButtons
. But I'm kinda iffy on passing a prop just for automation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
data-paprika-type="RawButton"
{...moreProps}
should be in charge of adding data-qa-anchor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's an example:
qaAnchor="paprika-popover--trigger" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like the qaAnchor approach you can totally do:
<RawButton data-qa="some stuff" /> and be easier.
the reason I like data-paprika-type="RawButton" is that you can select it when you are composing a component and not only for testing.
outline: none; | ||
} | ||
|
||
${props => (props.isDisabled ? disabledStyles : null)}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, I meant to change this to:
${props => props.isDisabled && disabledStyles};
I think it will work, and is a bit more concise. Unless there's a cleverer way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually think the current version is better. We don't need to be concerned what happens if it resolves to ${props => false};
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand... are you thinking it would actually try to print false
in the css?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. I don't know and the reader of the code should not need to worry about this I think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It works without any problem. I do like typing less... but you really think an explicit null
for falsey
cases is better?
@@ -1,7 +1,7 @@ | |||
import React, { Component } from "react"; | |||
import { text, number, select } from "@storybook/addon-knobs"; | |||
import styled from "styled-components"; | |||
import { CenteredStory } from "../Popover.stories.styles"; | |||
import { CenteredStory } from "../../../../.storybook/assets/styles/common.styles"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know we are using like packages and stuff, but is there a way to not have crazy paths like these? Maybe set some clever alias in webpack?
import { storiesOf } from "@storybook/react"; | ||
import Basic from "./examples/Basic"; | ||
|
||
storiesOf("RawButton", module).add("Basic", () => <Basic />); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we plan to flesh out the story more? Or is it part of another ticket / issue ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it really needs anything more... except testing stories.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we have a discussion about the storybook experience in general. My gripe with ACL UI's storybook is it's usually impossible to know how to use a component just by looking at its stories, without opening up the repo and actually viewing the component code. (even with addon-info, the story source shown in the browser is most often not complete enough)
Although do let me know if it's not supposed to be a supported use case π
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We did decide to try to use Storybook for documentation as much as possible, and being able to quickly view some example source code so you can understand how to use a component, is a legit use case and important piece of a much larger initiative that we will tackle with Paprika.
cc @oscarkwan
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah we need to research more into component documentation. I'm not sure if storybook is the right solution for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
readme.md
per component?, I'm in the same boat I have never been able to use storybook as a documentation tool. But let's wait for the documentation research. documentation is not the highest priority for the first stretch.
@@ -1,12 +1,8 @@ | |||
import React, { Component } from "react"; | |||
import { node, func, oneOfType } from "prop-types"; | |||
import styled from "styled-components"; | |||
import { PopoverContext } from "../../Popover"; | |||
import RawButton from "@paprika/rawbutton"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should stick to kebab-case for package names (so, raw-button
). Otherwise it'll get ugly with longer names e.g. outsideclickwrapper
.
Not sure if we want to do the same for packages directories. Wouldn't it be easier to just have the same name for the directory and the package?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can packages be CamelCase?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm finding some information that npm package names should be all lowercase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can modify that in another PR so we don't modify the popover folder, I'm fixing a lot of eslint error on it.
@@ -1,7 +1,7 @@ | |||
import React, { Component } from "react"; | |||
import { text, number, select } from "@storybook/addon-knobs"; | |||
import styled from "styled-components"; | |||
import { CenteredStory } from "../Popover.stories.styles"; | |||
import { CenteredStory } from "../../../../.storybook/assets/styles/common.styles"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is only ever run under storybook's webpack, we can add an alias for .storybook
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh! Of course!
packages/RawButton/RawButton.js
Outdated
/* eslint-disable jsx-a11y/no-static-element-interactions */ | ||
/* eslint-disable jsx-a11y/click-events-have-key-events */ | ||
/* es-lint is disabled for these specific reasons: | ||
1. Click-events-have-key-events: we are not using React synthetic events because we can't |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would review this. In terms of the comment, scrolling is prevented with e.preventDefault()
, not e.stopPropagation()
. For the actual code, if normal onKeyDown
is too late to prevent it, we could use onKeyDownCapture
.
packages/RawButton/RawButton.js
Outdated
export default class RawButton extends React.Component { | ||
constructor(props) { | ||
super(props); | ||
this.$rawButton = props.buttonRef || React.createRef(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So it sounds like there's two schools of thought.
<RawButton ref={someRef} />
where someRef
is forwarded to some inner DOM element with React.forwardRef
. By virtue of being called ref
it's transparent that you're putting a ref on a React component (RawButton
), not an actual DOM element. This assumes no one will ever need to grab a ref for the RawButton
itself.
<RawButton buttonRef={someRef} />
The "old" way. styled-components itself does this with innerRef
.
Thoughts?
NB. if we do props.buttonRef
in the constructor, we also need to handle someone swapping it for a different value in componentDidUpdate
(would be much easier with hooks π)
packages/RawButton/RawButton.js
Outdated
handleKeyDown = event => { | ||
if ( | ||
// Prevent scrolling the page with a spacerbar keypress | ||
(event.key === " " && event.target.tagName !== "INPUT") || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd do a case-insensitive comparison for tag name (e.g. by lowercasing both operands)
packages/RawButton/RawButton.js
Outdated
) { | ||
event.preventDefault(); | ||
} | ||
if (this.props.isDisabled || (!this.props.canPropagate && event.target !== this.$rawButton.current)) return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Long line.
packages/RawButton/RawButton.js
Outdated
}; | ||
|
||
render() { | ||
// eslint-disable-next-line no-unused-vars |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's see if we can use ignoreRestSiblings
option https://eslint.org/docs/rules/no-unused-vars#ignorerestsiblings
} | ||
} | ||
|
||
RawButton.displayName = "RawButton"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not necessary IMO
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Won't it get bungled in transpilation / bundling making it more difficult to debug?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In production, yes. Here's Dan Abramov's opinion: facebook/create-react-app#4081 (comment)
For a library, I think we should not be making this decision on behalf of the consumer. If they really need to, I think they can import our component and put a displayName on it manually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, I think we are the primary consumer we should be concerned with. Since the main drawback is bloat, and I don't think it's going to amount to much of that, we should do what will be most convenient for us. Then again, how often are we debugging on production?
cc @nahumzs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will go with Mike in this one, I don't think we can really bloat or component for some character, and even if the consumer wants to change the displayName they can, the could override the current displayName.
"prop-types": "^15.6.2" | ||
}, | ||
"peerDependencies": { | ||
"react": "^16.4.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we clean up the mismatching versions? This will get multiplied as people add more stuff.
And also decide on which versions we support
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really part of this PR, but why not 16.7.0
? Why would we launch a brand new repo without having all the latest stuff?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because we're making it a hassle for people to consume us: #25 (comment)
IMO
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm guessing it's not that critical for minor versions, but it still holds. This is similar to writing node libraries. If I'm targeting node 10, and my consumers are on node 8, they will not immediately be able to consume me, and I'm making their migration to node 10 an even bigger challenge (because now they need to handle everything that the migration involves, plus installing and using my library).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
interesting this is working, as far I know we set Lerna into 16.3.2
for react
, next week version will include hooks, I think we should target that one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't see your comment alex before commenting, but I think by the time we release and we start sharing the library the latest react version that we are picking right now, will be a regular version by that time. Is not a bad idea to go with the latest one right now IMO
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We ourselves are using a version from 2 years ago and it's unclear to me when this is going to change, so we've already set up a barrier for entry even for ourselves by going to 16.
https://github.com/facebook/react/blob/master/CHANGELOG.md#1542-january-6-2017
If I'm in the minority, I will defer to UX on this, but I'm not convinced. We all love the newest cool stuff, but my ideal component library would have something like this: https://github.com/palantir/blueprint/blob/9a7d665be381cc65e61e605d24da34c23dcce145/packages/core/package.json#L55
<p>Freegan squid pug heirloom letterpress pork belly, readymade you probably havenβt heard of them.</p> | ||
<p> | ||
<RawButton | ||
ariaText="ceci n'est pas un button" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
un bouton
π
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CΓ’lisse, c'est vrai.
packages/RawButton/RawButton.js
Outdated
const defaultProps = { | ||
ariaText: null, | ||
buttonRef: null, | ||
canPropagate: false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like the default should be true
because that's how normal buttons work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I think we often don't want that behaviour, but within our library we can set it to false
when we need to.
@@ -441,3 +406,5 @@ Popover.Trigger = Trigger; | |||
Popover.Content = Content; | |||
Popover.Card = Card; | |||
Popover.Tip = Tip; | |||
|
|||
export default Popover; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I decided I didn't like the inconsistency of having some default exports at the bottom for functional components, and up above for class components. Put them all at the bottom.
|
||
RawButton.displayName = "RawButton"; | ||
|
||
RawButton.propTypes = propTypes; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, it's the third option I've seen how to define prop types π
Was this decision discussed somewhere? Any pros comparing with defining types at the end of a file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will vote to go with propTypes at the top and with Static unless is a functional component. We haven't decided officially, but I think we should do it before continue migrating and adding the new components.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, @nahumzs and I discussed this when writing the <Popover>
, though I guess he was not entirely convinced π
Having propTypes
at the top makes sense since they define the API which is probably the first thing you want to know about a component. However, you can't define static
propTypes
in a functional component, so you could sometimes put them at the top for classes
and other times would have to put them at the bottom. I really don't like the inconsistency of that. More than seeing the API first, I want to know where to find it when I open a component.
So I proposed this unconventional approach that gives us the API at the top, consistently for Class components as well as functional ones. This is a brand new repo, and we have the opportunity to do things different... if we all agree to it.
Do we agree that getting consistency and an API at the top of the file is worth introducing an unconventional pattern?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should also mention this is the approach advocated by airbnb
https://github.com/airbnb/javascript/tree/master/react#ordering
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't even know that! Great minds.... π
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if it was you or Nahum, but I definitely remember referring one of you to their style guide for this a few months ago π
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's go with that approach!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 from Flatstack
|
||
render() { | ||
// eslint-disable-next-line no-unused-vars | ||
const { ariaText, buttonRef, canPropagate, children, isDisabled, tabIndex, ...moreProps } = this.props; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like we can leave children
inside the moreProps
.
packages/RawButton/RawButton.js
Outdated
<RawButtonStyled | ||
{...moreProps} | ||
aria-disabled={isDisabled} | ||
data-paprika-type="RawButton" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please clarify why do we need this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in testing, when you want to target a <button />
or a <input />
is quite simple, but since this is a button using a <span />
is quite challenging sometimes.
this allowed you to do document.querySelectorAll("[data-paprika-type='RawButton']")
and target multiple or just one rawbutton easily.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's what I think about how RawButton
should look like:
-
It must not has any additional attributes that are used only for testing purposes.
RawButton
already hasrole="button"
attribute which can be used to find a button on a page, e.g. when you write an acceptance test using Capybara. In tests, you likely want to find just a button, not exactly a button rendered byRawButton
React component. If you know a case when you need exactly aspan
rendered byRawButton
component and there is no other way to find it in DOM, please let me know. -
We should not use
span
tag forRawButton
because:
2.a. It's semantically wrong.span
is just an inline container. ButRawButton
represents a clickable element. https://developer.mozilla.org/en-US/docs/Web/HTML/Element/span
2.b. We violate:2. no-static-element-interactions: It's highly recommended to not use DIV or SPAN for interactive elements
.
2.c. We have to complicateRawButton
component with additional logic of makingspan
behave likebutton
. For example, explicitly definingaria-disabled
,tabIndex
attributes. We don't need to do it if<button>
is used instead ofspan
.
2.d. It makes writing tests more difficult. You can't simply typeclick_button "Save"
using built-in Capybara helpers. -
We are doing this because using a <button /> normally inherents css styling from frameworks such as bootstrap or foundation, so in this way we always have a fresh button with no style attached.
IMO, usingspan
just because it has no any styling defined by Bootstrap, Foundation, any browser, is not a correct way to solve the problem. We can't be 100% sure that some module doesn't have styles forspan
that can break everything. For example. Projects has https://github.com/acl-services/workpapers/blob/b4d6ed47cc14cb851a74bdad28517470770292dc/app/assets/stylesheets/core/content/_header.scss#L75.
I think that we have to explicitly define styles for button
to make it look the same in each module. If Bootstrap does button { margin-bottom: 5px }
then we have to set margin-button: 0
in the RawButtonStyled
. If e.g. some Foundation
does button { padding-top: 5px }
then we have to set padding-top: 0
as well. Then looking at RawButtonStyled
you will know how exactly the component should look like.
If you think that that's not enough, i.e. some Bootstrap defines stronger styles like body .control-group button { margin-bottom: 5px }
, or if it's not so good to have CSS rules defined in RawButtonStyled
just to fix styles for one case in some module, then we can make paprika
to provide some helper which will reset all styles for buttons. Then if Projects wants to start using RawButton
from paprika
but it has some legacy CSS for button
then we can fix it in Projects like so:
@import `~paprika/...`
button {
@extend reset-button-styles;
}
If one day Projects get rid of Bootstrap, we will only need to delete this fix. paprika
will remain untouched.
Please let me know what you think about this approach. I'd appreciate any feedback. Thanks!
CC @mikrotron, @nahumzs, @oscarkwan, @alexzherdev, @arkadiybutermanov
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @KirillKayumov, these are valid points, and while we agree with you for all three, I am still going to make arguments against for most of them. @nahumzs, @alexzherdev and I discussed your concerns to make sure we (mostly) have consensus among the three of us on this.
-
We agree that the
data-paprika-type='RawButton'
is not really necessary. We would like to continue using adata-qa-anchor
attribute to target specific<RawButtons>
for testing, but this will not be built into the<RawButton>
component, it will just be propagated by...moreProps
. -
Again, we agree that it is better to utilize a
<button>
element than attaching click handlers to a<span>
or<div>
. We shouldn't do this. But we also believe it is more practical in some cases to to use a generic<span>
, especially in our current apps, and sometimes in our library.
- The semantic meaning is conveyed in the JSX source code, and is not critical for the generated source βΒ as long as we specify
role=button
, the browser will treat the<span>
as clickable. - It's true that we violate some lint rules, but those can be disabled for this component, since we are doing it deliberately.
- Yes, we do have to do the extra work of handling aria attributes, and keyboard events, but we only have to do them once in this component, and then anytime we need to make a generic
<span>
element clickable, we can use this component. This has helped us with components like the<Multiselect>
where the list items and trigger are buttons, but not styled<buttons>
.
There are other examples, but perhaps the most convincing is from WAI Aria (Example 3) where they've opted not to "unstyle" a<button>
and applyrole=button
to a<div>
instead:
https://www.w3.org/TR/wai-aria-practices/examples/combobox/aria1.1pattern/listbox-combo.html
-
I think resetting the styles for the
<RawButton>
or for all<buttons>
becomes problematic and is not an ideal solution either. It can add quite a bit of bloat to the component, but also necessitates a more specific CSS override for app consumers. If I specify a CSS rule forbutton.pka-btn
, then later want to override it,.my-app .pka-btn
or.my-app button
will not be specific enough.
We tried a similar approach with Dashboard, Assurance Plans, etc by using areset.scss
stylesheet to undo a lot of global app styling:
https://github.com/acl-services/workpapers/blob/integration/client/app/containers/Dashboard/components/dashboard-reset.scss
And while it helped, it also caused problems later and we had to use some pretty ugly CSS overrides of the overrides. This one is not so bad, but I couldn't find some of the uglier ones I have had to do in these apps.
https://github.com/acl-services/workpapers/blob/integration/client/app/containers/Dashboard/components/TaskTable/TaskTable.scss#L5
In the Bootstrap example ofbody .control-group button
, this would be more specific thanbutton { @extend button-reset; }
, so I think we'd continue to run into this problem.
The<span>
solution is appealing, because we are not solving the CSS problem by adding more CSS (an approach to be avoided, in my opinion).
And finally, yes, aspan
ordiv
can have global styling applied to it too, but this is not very common, at least at a global level. -
All that being said, we would still prefer to use a
<button>
where possible. That's not really the purpose of the<RawButton>
component, but we plan to use anisSemantic
prop for the upcoming<Button>
component, that could betrue
by default, and would produce a<button>
with styling applied. However, ifisSemantic={false}
is provided, then the<Button>
would utilize the<RawButton>
(as the current<Button>
component does) and output a<span>
that would be insulated from global CSS targetingbutton
. We would discourage that though, and in any new apps that don't have Bootstrap / Foundation, we could start using the semantic version.
This is a bit similar to the way we have implemented the<Heading>
component:
https://github.com/acl-services/acl-ui/blob/master/src/components/Heading/Heading.js#L57 -
For a bit more context, even though Paprika is a generic UI library, in these early days our focus is on primarily allowing us to consume these components for our use cases. If we get to sidetracked by "what ifs" that arise from thinking about other potential consumers, we could spend a lot more time and energy than we realistically have available. We don't want to paint ourselves into a corner and make bad design decisions that will be difficult to change later, but we want to stay as focussed as we can, and if we have to introduce breaking changes later to make the library more generic, that's okay.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mikrotron thank you for the detailed answers!
- Ok, sounds good.
2-4. Your explanations made me think about... What if we need two separate components:
- Represents a button.
- Represents a clickable (interactive) element.
Examples:
-
Regular "Save" button in a form. It's weird to see that actually it's a
span
. And this is because we useRawButton
insideButton
acl-ui component. What if we implementButton
component here inpaprika
from scratch using<button>
and withoutRawButton
? Yes, we will need to share some common logic withRawButton
but this is another challenge. There are many ways to solve that. -
Collapsible
. Here we just have a text (possibly with collapse/expand icons) which is interactive. And I may agree that<button>
is not a best choice in this case. Some component likeRawButton
which uses<span>
and just wraps an element to make it interactive suits better. Then maybe it's worth to renameRawButton
to something likeInteractive
? Because in my opinion,RawButton
lets you think that you work with a button but maybe without any styling or whatever you think when you see "raw" word. But you get spans and like "What? Why spans?" -
Toogle/ToogleItem
. This case is not so obvious. I'm not 100% that we have to use<button>
here. From the one hand it looks like a button, from the other hand it's just an interactive element which may contain more just simple text.
WDYT about all of this?.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, had the same thoughts as Kirill ^ The idea was to rename RawButton
to something behavior-explaining like Clickable
or Interactive
, etc. That way it will be a lot less confusing for consumers.
Just a note about isSemantic
prop - I'm not sure how you feel about it, but for me personally this is one of the weirdest thing I've ever seen :) I think, the existence of this prop pretty clearly indicates, that we are doing something wrong. Looks like we are trying to solve some problem in a wrong direction (masking it instead of fixing the root cause).
Also, as I can see in this discussion, there are some non-obvious reasons for some solutions, and all this information is currently keeped in the heads of developers. Do you think we should reflect that in some FAQ? I'm pretty sure, each new developer joining the team asks the same questions like: "Why button is not button?" and "Why component can be intentionally not semantic?".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mikrotron This is just a question about the naming of the data qa attribute.
We haven't really committed to a testing library / approach to paprika yet. But Kent's react-testing-library
and cypress-testing-library
utilizes getByTestId
which requires components to have data-testid
as the name for it to work.
Perhaps we can use this instead? If it's for testing, we should name it for testing and not a generic paprika-type
attribute IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@oscarkwan That's a good suggestion. I'm a little hesitant about using id
which suggests that it must be unique, when in many cases it might not matter if you have another with the same id
somewhere, in fact you might even want an array of items to all have the same id
for convenience. Maybe data-test-name
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@KirillKayumov, @arkadiybutermanov I think we are getting much closer to consensus here!
We do envision two components, a <Button>
that will be used for actual skeuomorphic buttons and a <RawButton>
(or <Clickable>
or something else maybe) for encapsulating all of the a11y
implementation required once you add role='button'
to a <span>
. For the <Button>
we would prefer it to output a <button>
, but for pragmatic reasons, we'll still want the option to apply our styling to a <span>
.
@arkadiybutermanov is right that we are avoiding the root cause here, however the solution to that root problem is far too expensive (and impractical considering much of the effort would be in areas or our app that will be replaced in the future). We've made some efforts in Strategy to remove Bootstrap, but even there we hit some limits, and it's a much smaller app than Projects. Results and Visualizer would also have issues where they use Foundation UI components, and so we have conceded that we will need to live with these dependencies for a while. However, for new apps, and even new features that can stand alone, we should be able to avoid those frameworks and use the same Paprika <Button>
component, but allow it to generate a <button>
element.
In the meantime, we want to use Paprika components in our existing apps alongside Bootstrap and/or Foundation, so this idea of a <RawButton>
or <Clickable>
component is a cheap way to do so that has proven quite easy and effective so far. We agree it probably needs a better name than <RawButton>
, as this is clearly suggesting it's an actual button... π€
As for the problem of "all this information is currently kept in the heads of developers" that's definitely a problem we have now and want to get better at with Paprika. For now it's still early days, but we definitely need to expose more of the information in writing for all consumers to easily access and understand. Easier said than done, but documentation is definitely a core item on our list for the Paprika project.
packages/RawButton/RawButton.js
Outdated
|
||
componentDidMount() { | ||
if (this.$rawButton) { | ||
this.$rawButton.current.addEventListener("keydown", this.handleKeyDown); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have to define event listeners using addEventListener
only because of the 1. Click-events-have-key-events
comment above?
I've tried to use normal onKeyDown
and page wasn't scrolled when space key pressed.
<RawButtonStyled
{...moreProps}
onKeyDown={this.handleKeyDown}
/>
packages/RawButton/RawButton.js
Outdated
if ( | ||
// Prevent scrolling the page with a spacerbar keypress | ||
event.key === " " && | ||
event.target.tagName.toLowerCase() !== "input" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we render an input inside a RawButton somewhere? π±
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was a patch for the platform somewhere, I don't remember where specifically :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd recommend fixing the problem in that place instead of patching RawButton
and making it complicated just to handle one edge-case in some part of the platform.
If tomorrow we have a textarea
rendered in a raw button, hardly we will be happy to update his condition like so:
event.key === " " && event.target.tagName.toLowerCase() !== "input" && event.target.tagName.toLowerCase() !== "textarea"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was to fix an issue with the <MultiselectWithTags>
component where the <Trigger>
(which was implemented as a <RawButton>
) which opens the <Popover>
of list items, contained an <input>
(for custom tags). So there was an <input>
inside a <RawButton>
. It would be better to stopPropagation()
in the actual <input>
though, so I will remove this code, it shouldn't be necessary.
packages/Popover/package.json
Outdated
@@ -15,7 +15,7 @@ | |||
"dependencies": { | |||
"@paprika/tokens": "^0.0.1", | |||
"lodash.throttle": "^4.1.1", | |||
"prop-types": "^15.6.2" | |||
"prop-types": "^16.4.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you actually try yarn
ing? π prop-types
only goes up to 15.6.2 https://www.npmjs.com/package/prop-types
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
π€¦ββοΈ
packages/RawButton/RawButton.js
Outdated
|
||
componentWillUnmount() { | ||
if (this.$rawButton) { | ||
this.$rawButton.current.removeEventListener("keydown", this.handleKeyDown); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a bug here.
- open storybook (make sure you just open
localhost:9009
without any further path) - switch to the rawbutton story
- observe
Cannot read property 'removeEventListener' of null
at this line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should disappear when we use React events.
β¦yled components
β¦mports; kebab-case package name; default exports at bottom; no need for qaAnchor prop; setRef in componentDidUpdate();
β¦ixes; Update Storybook story;
efe546e
to
f869496
Compare
@@ -33,37 +33,37 @@ const throttleDelay = 20; | |||
|
|||
const propTypes = { | |||
/** Where the popover content is positioned relative to the trigger or getPositioningElement. */ | |||
align: oneOf(["top", "right", "bottom", "left"]), | |||
align: PropTypes.oneOf(["top", "right", "bottom", "left"]), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shoudln't we use import { oneOf, node, ...etc } from "prop-types"
inste of PropTypes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That way isn't in line with the es6 spec, from what I understand.
#32 (comment)
There's also the argument that it's annoying to have to update that import all the time (same reason Kent Dodds argues for using React.Hookname
instead of import { Hookname } from React
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like it but it's the way to go is ok for me.
import RawButtonStyled from "./RawButton.styles"; | ||
|
||
const propTypes = { | ||
ariaText: PropTypes.string, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No action item right now but I wonder if we need this one, we can do it automatically for our developers, we can check if prop.children is a string and assigned directly to aria-text
, if is not a string, we can ask the developer to add a label to the and use that label to describe the content of the RawButton and assigned to aria-text.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
If no
aria-label
is used on a button, the screenreader itself will use whatever string is contained inside the element, so we don't even have to do anything. So this prop is only for when the content is an icon or lacks context so anaria-label
that is more descriptive can be provided. -
I'm proposing the use of an
ariaText
prop to insulate app developers from the a11y implementation details. Whether that value gets used by simply passing it through to anaria-label
on the root node, or in a more complex component, maybe anaria-labelledby
gets used, or it's applied to a specific node that's not the child, or for a<table>
it actually gets applied to a<caption>
element... the app developer doesn't need to care. They can follow the consistent convention of using theariaText
prop. Not to be confused with thearia-text
attribute, because that attribute doesn't exist.
|
||
const propTypes = { | ||
ariaText: PropTypes.string, | ||
buttonRef: PropTypes.shape({ current: PropTypes.instanceOf(Element) }), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this will not need it we should do it with forwardRef, this can be done in another PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Purpose π―
Add a
<RawButton>
component as an accessible button that is resilient to global CSS.Notes βοΈ
<Button>
: UX-433 Migrate RawButton πΒ #32 (comment)<Popover>
stories to use them too.<RawButton>
to<Popover.Trigger>
.Screenshot πΈ