Skip to content
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

build(opentrons): add styled-components to buildtools #5129

Merged
merged 6 commits into from
Mar 3, 2020

Conversation

iansolano
Copy link
Contributor

@iansolano iansolano commented Feb 28, 2020

Add styled-components and additional buildtools in support

closes: #4866

Added to the cookbook here: https://coda.io/d/Front-end-Cookbook_dw3eEFMgHPr/Styled-Components_su1oc#_luNNQ

overview

changelog

review requests

  • Test card component is styled and renders correctly in Opentrons Components Style Guide

Add styled-components and additional buildtools in support
@iansolano iansolano requested review from a team as code owners February 28, 2020 22:53
@codecov
Copy link

codecov bot commented Feb 28, 2020

Codecov Report

Merging #5129 into edge will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             edge    #5129      +/-   ##
==========================================
- Coverage   68.86%   68.85%   -0.02%     
==========================================
  Files        1082     1089       +7     
  Lines       36042    36245     +203     
==========================================
+ Hits        24821    24957     +136     
- Misses      11221    11288      +67     
Impacted Files Coverage Δ
opentrons/server/__init__.py 73.49% <0.00%> (-5.73%) ⬇️
opentrons/main.py 35.92% <0.00%> (-2.72%) ⬇️
protocol-designer/src/file-types.js 0.00% <0.00%> (ø) ⬆️
protocol-designer/src/components/ProtocolEditor.js 0.00% <0.00%> (ø) ⬆️
components/src/styles/colors.js 100.00% <0.00%> (ø)
...mponents/modals/AnnouncementModal/announcements.js 100.00% <0.00%> (ø)
components/src/styles/typography.js 100.00% <0.00%> (ø)
...r/src/components/modals/AnnouncementModal/index.js 100.00% <0.00%> (ø)
components/src/styles/spacing.js 100.00% <0.00%> (ø)
components/src/styles/font-weight.js 100.00% <0.00%> (ø)
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 89fe8bd...95af408. Read the comment docs.

@@ -50,9 +50,11 @@
* non-standard, but works on all webkit browsers, Edge, and Firefox >= 68
* it turns out CSS ellipses for multiline blocks are hard
*/
display: -webkit-box; /* stylelint-disable-line declaration-block-no-duplicate-properties */
/* stylelint-disable */
display: -webkit-box;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changes required ignoring all three webkit prefixes

@Kadee80
Copy link
Contributor

Kadee80 commented Mar 2, 2020

@iansolano can you add in a simple example or two of what porting a simple component to styled components might looks like for context? Card and LabeledValue seem like nice isolated components to start with so we can all grok what making these changes might entail.

Copy link
Contributor

@mcous mcous left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple little housekeeping requests, but otherwise this is great! Very excited to start using this.

Also, like @Kadee80 said (and discussed in the tech debt meeting), additions to the Front-end Cookbook for basic how to use info on styled-components (plus copy-paste snippets) I think are needed to call the RFC closed

babel.config.js Show resolved Hide resolved
@@ -50,9 +50,11 @@
* non-standard, but works on all webkit browsers, Edge, and Firefox >= 68
* it turns out CSS ellipses for multiline blocks are hard
*/
display: -webkit-box; /* stylelint-disable-line declaration-block-no-duplicate-properties */
/* stylelint-disable */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we disable the specific rules rather than every rule?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure


return (
<section className={style}>
{title && <h3 className={styles.card_title}>{title}</h3>}
<Section disabled={disabled} className={className}>
Copy link
Contributor Author

@iansolano iansolano Mar 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keeping className allows folks to easy extend the component styles doing:

const NewCard = styled(Card)`
  color: red; // extended style
`

@@ -2,10 +2,10 @@

exports[`Card renders Card correctly 1`] = `
<section
className="card"
className="Card__Section-sc-1ltsa6e-0 fUnCuR"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking: we said we're planning to not use snapshot tests, but maybe in the meantime we should use https://github.com/styled-components/jest-styled-components#snapshot-testing to make these more readable?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this PR already pulls in this dependency, so it should be a matter of placing that import call in a test setup script (see scripts for global enzyme and mocks setup)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

import 'jest-styled-components' in the test file should do the trick and is already included as mike mentioned.

Copy link
Contributor

@mcous mcous left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯

@iansolano iansolano merged commit 69f82f3 into edge Mar 3, 2020
@iansolano iansolano deleted the ot_add-styled-components branch March 3, 2020 20:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RFC: Introduce styled-components to monorepo
4 participants