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

React 16 upgrade #406

Merged
merged 64 commits into from
Apr 11, 2018
Merged

React 16 upgrade #406

merged 64 commits into from
Apr 11, 2018

Conversation

alexreardon
Copy link
Collaborator

@alexreardon alexreardon commented Mar 25, 2018

There is still a lot to do on this one. But I am starting the PR early. Closes #406 #192 #103 #8

  • Upgrade react dependencies
  • Upgrade enzyme and ensure all tests are passing
  • Fix linting
  • Convert placeholder to use the same element type as the element dragging
  • Move to portals
  • Thrash out new placeholder api
  • Update docs and examples to use new Draggable api
  • Add portal example
  • Add a portal guide
  • Add tests for post portal movement auto focus (and focus publishing)
  • add tests for new behaviour (if any)
  • Ensure all tests are passing
  • Build sortable table example
  • Fix linting
  • Fix flow
  • Write table sorting guide
  • Update all stories and docs to use new api
  • Write release notes (including perf gain table 😲)

In scope?

  • Moving to React 16.3
  • Using the new React 16.3 lifecycle methods (or at least moving off the deprecated ones)
  • Internally move to the new Context api

Out of scope

Currently stating min version of React 16.3. Should this be increased to 16.3? At this stage I am not interested in being slowed down by trying to be compatible with older React versions 🤔

@alexreardon alexreardon mentioned this pull request Mar 29, 2018
@tim-soft
Copy link

tim-soft commented Apr 5, 2018

I can't imagine many situations where someone may be "stuck" from upgrading from React 16.2 -> 16.3.

That upgrade path is a much smaller burden on the user than the bottleneck of being backward compatible. If you are looking for opinions I say go all the way ¯\_(ツ)_/¯

@alexreardon
Copy link
Collaborator Author

Agreed @tim-soft

@alexreardon
Copy link
Collaborator Author

alexreardon commented Apr 5, 2018

I have moved everything over to React 16.3 including moving away from the deprecated lifecycle methods. Making progress!

About 5% of tests are broken. Not too bad to start from!

screen shot 2018-04-05 at 3 14 33 pm

draggableId={draggableId}
droppableId={droppableId}
index={index}
targetRef={this.state.ref}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sweet advantages of this: no longer need a double render :D

@@ -7,8 +7,8 @@ import { quotes, getQuotes } from './src/data';
import { grid } from './src/constants';

const data = {
small: quotes,
// small: getQuotes(3),
// small: quotes,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

TODO: undo

Copy link

@petegleeson petegleeson left a comment

Choose a reason for hiding this comment

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

Overall looks good. Just a few concerns about todos in docs.

| Can copy paste the table into other applications | Browser |
| Can reorder items in the table! | `react-beautiful-dnd` 😎|

`react-beautiful-dnd` supports requires no additional wrapping elements to create `Draggable` and `Droppable` components. Therefore it is possible to have a `<table>` that has valid HTML as well as supporting drag and drop.

Choose a reason for hiding this comment

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

"react-beautiful-dnd requires no additional"


The only thing you need to do is set `display: table` on a `Draggable` row while it is dragging.

[See example code here](TODO)

Choose a reason for hiding this comment

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

Do we have code samples for here, here and here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We do now! 👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll update the links

@@ -8,7 +8,7 @@ import { grid } from './src/constants';

const data = {
small: quotes,
// small: getQuotes(3),
// small: quotes.slice(0, 2),

Choose a reason for hiding this comment

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

can we remove this?

}

render() {
// console.log('quote item rendered', this.props.quote.id);

Choose a reason for hiding this comment

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

Can probably remove the changes to this file

|}
class TableCell extends React.Component<TableCellProps> {
// eslint-disable-next-line react/sort-comp
ref: ?HTMLElement

Choose a reason for hiding this comment

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

Can use ElementRef to type these variables.

Copy link
Collaborator Author

@alexreardon alexreardon Apr 10, 2018

Choose a reason for hiding this comment

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

These are actual HTMLElement and not component instances

if (!document.body) {
throw new Error('document.body required for example');
}
document.body.appendChild(table);

Choose a reason for hiding this comment

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

Will this add many tables because of HMR while in dev?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For now I think it is fine

@alexreardon
Copy link
Collaborator Author

Yeah it will. But I am not worried about it. You think it is worth cleaning up the portals?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants