-
Notifications
You must be signed in to change notification settings - Fork 479
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
Remove Radium from ScrollableList and ItemLoopToggle (Animation Tab elements) #47820
Conversation
onClick={this.toggleClicked} | ||
> | ||
<img src={iconImageSrc} style={styles.loopIconStyle} /> | ||
<img src={iconImageSrc} className={style.loopIconStyle} /> |
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 see loopIconStyle
anywhere (in previous version or new version), am I missing something?
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.
Huh, good point. I think I was on autopilot for this change and missed that. Seems safe enough to just remove this.
@@ -30,21 +30,15 @@ class ItemLoopToggle extends React.Component { | |||
return ( | |||
<OverlayTrigger overlay={tooltip} placement="bottom" delayShow={500}> | |||
<div | |||
style={[styles.loopToggleStyle, this.props.style]} | |||
style={{...this.props.style}} |
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.
Nit: style={this.props.style}
} | ||
|
||
.margins { | ||
margin: 4; |
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.
Nit: 4px? Does this work without specifying units?
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 most certainly did not, but it should now!
Maybe update PR title to describe what components you're removing Radium from? |
Whoops, great call! :) |
className={this.props.className} | ||
style={[staticStyles.root, this.props.style]} | ||
className={classNames(this.props.className, style.root)} | ||
style={{...this.props.style}} |
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.
Sorry one more destructuring nit I missed
This is part of the work to stop using Radium. See https://docs.google.com/spreadsheets/d/1chviZU39Qej9_xrc2lrcRrydKJKcs-W6Q0ehzVtH6bg/edit#gid=1128290845 for all components using Radium.
This PR includes changes to two animation tab components: ScrollableList and ItemLoopToggle.
Scrollable List:
ItemLoopToggle:
Links
Testing story
No regressions found. Compared styles for each styled element in the DOM.
Deployment strategy
Follow-up work
Privacy
Security
Caching
PR Checklist: