-
Notifications
You must be signed in to change notification settings - Fork 66
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
fix(asinput): Add danger theme to asInput component #91
Conversation
src/asInput/index.jsx
Outdated
...this.props.className, | ||
]} | ||
styles['form-inline'], | ||
{ [styles['is-invalid']]: !this.state.isValid && this.props.theme.includes('danger') }, |
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 no strong feelings about this theme's name. Any opinions?
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.
My only thought is that if we ever want to add additional styling in the future whenever !this.state.isValid
(like always making the background color red
or something) we'd probably want to name that style is-invalid
, right?
But even if we have to rename this class it won't be a breaking change (right?), so I don't think it's a big deal either way.
src/asInput/index.jsx
Outdated
<div className={styles['form-control-feedback']} id={errorId} key="0"> | ||
{this.state.validationMessage} | ||
|
||
<div className={classNames(styles['form-control-feedback'], { [styles['invalid-feedback']]: this.props.theme.includes('danger') })} id={errorId} key="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.
I can pull out the this.props.theme.includes('danger')
condition into a variable, as the condition is being used twice here, but I was interested to see whether it would be an issue as we add more themes. Is the idea that, as we add more themes, we conditionally apply classnames this way? I'll add a 'hasDangerTheme' variable if this looks good to everyone, otherwise.
src/asInput/index.jsx
Outdated
|
||
<div className={classNames(styles['form-control-feedback'], { [styles['invalid-feedback']]: this.props.theme.includes('danger') })} id={errorId} key="0"> | ||
{ this.props.theme.includes('danger') && | ||
<span |
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 is an alert icon, which follows UI/UX guidelines laid out here.
src/asInput/index.jsx
Outdated
@@ -89,15 +101,17 @@ const asInput = (WrappedComponent, labelFirst = true) => { | |||
render() { | |||
const { description, error, describedBy } = this.getDescriptions(); | |||
return ( | |||
<div className={styles['form-group']}> | |||
<div className={classNames(styles['form-group'])}> |
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.
Any harm in adding classNames?
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 there is any harm, but why do you think it is useful here if there are no class names to join?
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.
Ah, my bad. I had added class names and then removed them. I'll switch it back.
I'd be interested in thoughts about this "theme". It's a solution we discussed when considering the modal and styling that studio-frontend needed for the modal, but I haven't gotten around to it because of the accessibility form. Does this look like what was envisioned? I tried to use a similar rationale for this asInput theme. P.S. If anyone has the bandwidth, it would be great to get this merged soon, since it's needed for the Studio accessibility form that's due by December 12th (although we wanted to get it pushed out by mid-day Friday). |
src/asInput/index.jsx
Outdated
@@ -22,6 +24,7 @@ export const inputProps = { | |||
onBlur: PropTypes.func, | |||
validator: PropTypes.func, | |||
className: PropTypes.arrayOf(PropTypes.string), | |||
theme: PropTypes.arrayOf(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.
Naming nit: if this is an array of string
s should this be called themes
instead?
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 catch!
src/asInput/index.jsx
Outdated
@@ -49,9 +52,20 @@ const asInput = (WrappedComponent, labelFirst = true) => { | |||
const desc = {}; | |||
|
|||
if (!this.state.isValid) { | |||
const hasDangerTheme = this.props.theme.includes('danger'); |
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 the this.props.theme.includes('danger')
check is implemented in getDescriptions
and render
you could consider moving this to it's own helper method and that way if you ever have to adjust it / add a theme value, you can do it in one place instead of having to remember to do it in two places.
9993c2b
to
da6ab3e
Compare
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.
@MichaelRoytman this looks good - let's hold off merging until @arizzitano takes a look?
I know there is a bit of a time component here but I want to throw something out there:
There's a great post on Medium about lessons learned from doing code reviews. One of the lessons is to not use string
s when another type should be used (an enum
or a UUID
) - this is obviously more applicable for something like Java vs. Javascript. Here's the author's example gist.
There are two points to all of this:
- That the
themes
prop seems to be more like anenum
? Like we could have athemes
enum
in aconstants.js
where you can pick and choose the themes you want to apply via an interface like<Component... themes={[Theme.DANGER, Theme.UNDERLINE]} />
. I know this may seem like a lot of (potentially unnecessary) overhead, but I think it makes the API more accurate. It could also allows us to implement better prop type checking / demonstrate the intention of thethemes
prop by specifying a specificenum
/class
vs.PropType.string
. - It feels like we're trying to conflate two ideas - this idea of theming, and then this idea of supporting custom class names. They seem (and obviously are) coupled together - I can't tell if this will lead to some pitfall in the future, nor do I know what the right solution is right now, but I'm generally paranoid of exposing two API points that change the same piece of data (in this case, the component's class).
Edit: The more I think about the second point the less I'm worried - I think this is fine for now.
@arizzitano -- do you want to take a look? We are under time pressure, and if you intend to look at this, we'd appreciate if you could do so at your earliest convenience so that we can wire everything up. |
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 looks fine for now -- the enum vs. string concerns are probably not a big deal since the danger theme is only being used in this one place. If we want to change it we can circle back later.
@MichaelRoytman @dsjen hold on, I'm investigating if Update: here are the default release rules used by
Since |
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 the commit message should be fix(asinput): Add danger theme to asInput component
- this will guarantee that a new version of Paragon will be published to NPM (and thus, Studio can incorporate the changes).
Add danger theme to asInput component to add alert icon, red text for validation message, and red border to input element
da6ab3e
to
d545540
Compare
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.
After updating the commit message with @fysheets, I think this is ready to merge after the build succeeds.
Add danger theme to asInput component to add alert icon, red text for validation message, and red border to input element