-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
React 16 upgrade #406
Conversation
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 ¯\_(ツ)_/¯ |
Agreed @tim-soft |
draggableId={draggableId} | ||
droppableId={droppableId} | ||
index={index} | ||
targetRef={this.state.ref} |
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.
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, |
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.
TODO: undo
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.
Overall looks good. Just a few concerns about todos in docs.
docs/patterns/tables.md
Outdated
| 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. |
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.
"react-beautiful-dnd
requires no additional"
docs/patterns/tables.md
Outdated
|
||
The only thing you need to do is set `display: table` on a `Draggable` row while it is dragging. | ||
|
||
[See example code here](TODO) |
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.
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 do now! 👍
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'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), |
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.
can we remove this?
} | ||
|
||
render() { | ||
// console.log('quote item rendered', this.props.quote.id); |
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.
Can probably remove the changes to this file
|} | ||
class TableCell extends React.Component<TableCellProps> { | ||
// eslint-disable-next-line react/sort-comp | ||
ref: ?HTMLElement |
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.
Can use ElementRef to type these variables.
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.
These are actual HTMLElement and not component instances
if (!document.body) { | ||
throw new Error('document.body required for example'); | ||
} | ||
document.body.appendChild(table); |
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.
Will this add many tables because of HMR while in dev?
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.
For now I think it is fine
Yeah it will. But I am not worried about it. You think it is worth cleaning up the portals? |
There is still a lot to do on this one. But I am starting the PR early. Closes #406 #192 #103 #8
Move to portalsDraggable
apiIn scope?
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 🤔