-
-
Notifications
You must be signed in to change notification settings - Fork 143
Conversation
Would this require moving |
Those percy diffs look okay to me. Looks like a few components grew a pixel or two |
@rmarren1 Yeah exactly, the biggest difference is the DatePicker stuff, and indeed some of the Percy screenshots look off by a pixel or two (I noticed in the last snapshot the Dropdown component's text's vertical centering was a bit off). I don't think it requires upgrading dash-renderer's React, since there aren't really any breaking changes. Taken from the React docs, edit: |
acd64fd
to
5374277
Compare
5374277
to
3faabe0
Compare
@@ -1,3 +1,4 @@ | |||
import 'react-dates/initialize'; |
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 the new (or old, in any case the documented) way of using react-dates
, as is on the github page
We should either support package.lock and Can you remove |
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.
Please see comments 🙂
@@ -51,61 +51,61 @@ describe('Props can be set properly', () => { | |||
|
|||
test('props.id is set as the <input> id', () => { | |||
// test if id is in the actual HTML string | |||
const inputTag = input.render().children(); | |||
const inputTag = input.render(); |
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.
What's the difference between render()
and render().children()
? Does the change to the latter have to do with the React 16 upgrade?
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 has to do with the Enzyme upgrade.
@@ -118,15 +118,15 @@ describe('Input with (default) type=text', () => { | |||
input = mount(<Input value="initial value" />); | |||
}); | |||
test('Input updates value', () => { | |||
expect(input.find('input').getNode().value).toEqual( | |||
expect(input.find('input').instance().value).toEqual( |
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.
Similar question here, what is the difference between getNode()
and instance()
? An API change in enzyme
?
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.
Exactly. Enzyme got updated, and a bunch of the methods I used before didn't work anymore after upgrading.
} | ||
*, *:before, *:after { | ||
box-sizing: inherit; | ||
} |
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 a good idea in principle but it's a bit dangerous to set the wildcard selector. Other components might be expecting the default content-box
; we should double check if the layout looks different. I'd prefer if the above lines (6-11) were more explicitly in a "global" or "base" CSS file with the above comment explaining the rationale.
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.
On the other hand, there's minimal changes in the Percy diffs, so that's promising.
'height': '60px', | ||
'lineHeight': '60px', | ||
'borderWidatetimeh': '1px', | ||
'borderWidth': '1px', |
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.
👍
|
||
// Allows you to use a custom runner instead of Jest's default test runner | ||
// runner: "jest-runner", | ||
|
||
// The paths to modules that run some code to configure or set up the testing environment before each test | ||
// setupFiles: [], | ||
setupFiles: ['<rootDir>/test/setupTests.js'], |
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.
The Jest and Enzyme updates now require us to specify a React adapter to use - so we create this setupTests.js
setup file to run before our tests.
// "<rootDir>" | ||
// ], | ||
roots: [ | ||
"<rootDir>/test/unit" |
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 don't want to look in other folders for JS tests - specifically the new setupTests.js
file shouldn't be run as a unit test with Jest.
* ideally, we would use cloneElement however | ||
* that doesn't work with dash's recursive | ||
* renderTree implementation for some reason | ||
*/ |
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 for this noise - it seems Prettier
still picks up some formatting issues here and there from time to time. I think the formatting tests in Cirlce are maybe not 100% right, as it seems to miss these formatting issues.
@@ -1,4 +1,4 @@ | |||
import {arduinoLight, monokai} from 'react-syntax-highlighter/dist/styles'; | |||
import {arduinoLight, monokai} from 'react-syntax-highlighter/dist/styles/hljs'; |
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.
New syntax for updated react-syntax-highlighter
@chriddyp We could still merge this branch, since almost all of the packages used are updated. If dash-renderer provides React, and if the dcc bundles build with these updated packages/dependencies work without failing, then surely that won't break anything? I've also checked the updated packages's dependencies, and none specify that they don't work below React 16. If you're good with it, I can take out the references to version 1.0.0 and we can merge this, closing all of the open greenkeeper ones (which was the goal of this PR when I started with it anyway) |
Replace package-lock.json with yarn.lock in .circle/config
Reformat src and rebuild
Tiny react-dates styling fix Fix test regression CSS
e3c3343
to
3c74f5f
Compare
@Marc-Andre-Rivet Reverted the changes to the client-facing react packages such as react-dates, only internal tooling updates now. Again apologies for the messy PR / commit history! |
@Marc-Andre-Rivet Also removed |
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.
With the removal of the customer facing package updates (react-dates, markdown, highlighter, etc.) I see this essentially as a toolchain update. I understand that we are still running the integration/e2e tests against react 15.4.2 / the dash-renderer version, so the bump to react 16.6.1 only impacts the enzyme tests.
This feels very similar to the current situation in the dash-table where the table is technically coded against 16.6.x, has 0.14 peer deps, is tested against 15.4.2 and only tested against 16.6.x with @storybook/react
. The slight difference is that here the unit tests are run against 16 instead of 15 but integration/e2e tests should be able to catch a breaking change/usage.
lgtm
This PR updates most of the packages to their latest version.
This also removes some unused packages, like builder and old babel packages that are no longer needed.
Note that the move from react-dates 12.3 to 18.2 is a slightly bigger one - not only does it change the way styling is handled, but it also just looks different in general. I've updated the way we style our DatePicker components with some simple overwrites, but it still will look a little bit different. I don't think that's a bad thing though, necessarily. Check out the Percy diffs to see the changes in the DatePickers.