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

Upgrade to React 16 #508

Merged
merged 22 commits into from Apr 9, 2019

Conversation

Projects
None yet
2 participants
@Marc-Andre-Rivet
Copy link
Contributor

commented Apr 3, 2019

Fixes #428
Fixes plotly/dash-core#13

main pr, renderer: plotly/dash-renderer#145

  • Remove dash_table_experiment and replace by dash_table
  • DatePickerSingle & Range issues with React 16

Marc-André Rivet added some commits Apr 2, 2019

Marc-André Rivet
Marc-André Rivet
Marc-André Rivet
Marc-André Rivet
Marc-André Rivet
Marc-André Rivet
update react version
remove fragment polyfill
Marc-André Rivet
Marc-André Rivet
Marc-André Rivet
Marc-André Rivet
Marc-André Rivet
Marc-André Rivet
Marc-André Rivet
dt
Marc-André Rivet
Marc-André Rivet
Marc-André Rivet
@@ -27,7 +27,7 @@ jobs:
name: Install dependencies (dash)
command: |
git clone git@github.com:plotly/dash.git
git clone git@github.com:plotly/dash-renderer.git
git clone -b react-16 git@github.com:plotly/dash-renderer.git

This comment has been minimized.

Copy link
@Marc-Andre-Rivet

Marc-Andre-Rivet Apr 4, 2019

Author Contributor

To be removed once merged into master

@@ -1,6 +1,5 @@
import React from 'react';
import React, {Fragment} from 'react';

This comment has been minimized.

Copy link
@Marc-Andre-Rivet

Marc-Andre-Rivet Apr 4, 2019

Author Contributor

Fragment is supported by React 16, remove the polyfill

"react-dom": "^16.6.1",
"react-dot-fragment": "^0.2.8",
"react": "^16.8.6",
"react-dom": "^16.8.6",

This comment has been minimized.

Copy link
@Marc-Andre-Rivet

Marc-Andre-Rivet Apr 4, 2019

Author Contributor

Bump react, remove polyfill

@@ -1,6 +1,5 @@
percy
selenium
pandas
dash_table_experiments
xlrd

This comment has been minimized.

Copy link
@Marc-Andre-Rivet

Marc-Andre-Rivet Apr 4, 2019

Author Contributor

Changed table experiment for the table while at it -- the experiment was causing problems in any case

This comment has been minimized.

Copy link
@alexcjohnson

alexcjohnson Apr 5, 2019

Contributor

Just for the tests, or is the transition to 16 the end of the line for anyone still using experiments? Which would be fine, but it would mean we need to be very clear about this with users.

This comment has been minimized.

Copy link
@Marc-Andre-Rivet

Marc-Andre-Rivet Apr 5, 2019

Author Contributor

I changed this because I was having issues with the experiment. Will check if this is end-of-line or just usage details.

This comment has been minimized.

Copy link
@Marc-Andre-Rivet

Marc-Andre-Rivet Apr 5, 2019

Author Contributor

As discussed, will not looking further into this. The issue might be limited to the old import { PropTypes } from 'react' usage, maybe there's more to it.

test('DatePickerRange renders', () => {
const dps = render(<DatePickerRange />);
const dps = render(<DatePickerRange {...defaultProps} />);

This comment has been minimized.

Copy link
@Marc-Andre-Rivet

Marc-Andre-Rivet Apr 4, 2019

Author Contributor

These props are required when running the React dev build like in Jest. Otherwise console errors prevent the tests from passing.

start_date.send_keys("1997-05-03")
self.wait_for_text_to_equal('#date-picker-range-output', 'None - None')

# updated both dates, callback should now fire and update output
dt_length = len(end_date.get_attribute('value'))
end_date.send_keys(dt_length * Keys.BACKSPACE)

This comment has been minimized.

Copy link
@Marc-Andre-Rivet

Marc-Andre-Rivet Apr 4, 2019

Author Contributor

When used IRL the behaviour is ok (override existing text on click) -- but in Selenium this seems to be an issue now. Delete with backspace all characters..

dt_input_3.send_keys("1997-05-03")

dt_input_4 = self.driver.find_element_by_css_selector(
'#dt-range-no-date-values-init-month #endDate'
)
self.driver.find_element_by_css_selector(
'label'
).click()

This comment has been minimized.

Copy link
@Marc-Andre-Rivet

Marc-Andre-Rivet Apr 4, 2019

Author Contributor

Now that we are passing a valid value to react-dates, the calendar opens -- the alternative is throwing an error and crashing the render, not great.. all this means it's in the way when trying to select another component. Clicking something safe to close it.

dest[key] = null;

if (key === 'initial_visible_month') {
dest[key] = moment();

This comment has been minimized.

Copy link
@Marc-Andre-Rivet

Marc-Andre-Rivet Apr 4, 2019

Author Contributor

So apparently an invalid moment value is not good enough for initial_visible_month but is fine for everything else, doing moment() on the other hand uses the current month..

@@ -23,7 +23,7 @@ import Tab from './components/Tab.react';
import Store from './components/Store.react';
import LogoutButton from './components/LogoutButton.react';

import './components/css/react-dates@12.3.0.css';
import 'react-dates/lib/css/_datepicker.css';

This comment has been minimized.

Copy link
@Marc-Andre-Rivet

Marc-Andre-Rivet Apr 4, 2019

Author Contributor

As before, doing this here because Jest can't process css files for some reason that I don't feel like figuring out.

if (setProps && date !== null) {

this.setState({date});
if (date) {

This comment has been minimized.

Copy link
@Marc-Andre-Rivet

Marc-Andre-Rivet Apr 4, 2019

Author Contributor

setProps check is now usless

This comment has been minimized.

Copy link
@alexcjohnson

alexcjohnson Apr 4, 2019

Contributor

This whole if may be useless, if we accept #511

This comment has been minimized.

Copy link
@Marc-Andre-Rivet

Marc-Andre-Rivet Apr 5, 2019

Author Contributor

Yes! I saw the PR :)

Marc-André Rivet

this.setState({start_date, end_date});

if (start_date && !start_date.isSame(old_start_date)) {

This comment has been minimized.

Copy link
@Marc-Andre-Rivet

Marc-Andre-Rivet Apr 4, 2019

Author Contributor

.isSame, .isBefore, .isAfter are functions offered by moment to compare moment objects to various time formats

@@ -1,7 +1,8 @@
import 'react-dates/initialize';

This comment has been minimized.

Copy link
@Marc-Andre-Rivet

Marc-Andre-Rivet Apr 4, 2019

Author Contributor

Starting with react-dates 13+ that's required

Marc-André Rivet

@Marc-Andre-Rivet Marc-Andre-Rivet marked this pull request as ready for review Apr 4, 2019

@alexcjohnson
Copy link
Contributor

left a comment

💃 Glad to finally get this up to date!

Marc-Andre-Rivet and others added some commits Apr 8, 2019

Marc-André Rivet

@Marc-Andre-Rivet Marc-Andre-Rivet merged commit cc3ac2e into master Apr 9, 2019

4 checks passed

ci/circleci: python-2.7 Your tests passed on CircleCI!
Details
ci/circleci: python-3.6 Your tests passed on CircleCI!
Details
ci/circleci: python-3.7 Your tests passed on CircleCI!
Details
percy/dash-core-components No visual changes since last approval
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.