-
Notifications
You must be signed in to change notification settings - Fork 23
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
refactor: [DHIS2-17650] Replace Material-UI Table, TableBody, TableCell, TableHead and TableRow #3721
Merged
henrikmv
merged 17 commits into
master
from
hv/feat/DHIS2-17650_ReplaceMaterialUIComponentsInDragAndDropTableColumnSelector
Aug 12, 2024
Merged
refactor: [DHIS2-17650] Replace Material-UI Table, TableBody, TableCell, TableHead and TableRow #3721
Changes from 14 commits
Commits
Show all changes
17 commits
Select commit
Hold shift + click to select a range
862ec68
feat: change to dhis ui components
henrikmv af95c62
Merge remote-tracking branch 'origin/master' into hv/feat/DHIS2-17650…
henrikmv 68b3f87
fix: define drag source and drop target
henrikmv 5daf98e
fix: ts error
henrikmv 60cdfab
fix: ts error
henrikmv 65b4157
fix: ts
henrikmv 23c9b75
fix: restore comments
henrikmv e1b969f
fix: breaking cypress test
henrikmv b129920
fix: cypress
henrikmv 60303e2
fix: cypress test
henrikmv 974e207
Merge remote-tracking branch 'origin/master' into hv/feat/DHIS2-17650…
henrikmv c609f1a
fix: rolleback cypress change in fil
henrikmv 5b54ae3
Revert "fix: rolleback cypress change in fil"
henrikmv 3259138
fix: cypress
henrikmv 60ae38e
fix: review change for opacity
henrikmv 3002d39
fix: review change for hover
henrikmv 474b719
fix: merge conflict
henrikmv File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
142 changes: 52 additions & 90 deletions
142
...apture-core/components/ListView/ColumnSelector/DragDropList/DragDropListItem.component.js
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,107 +1,69 @@ | ||
| // @flow | ||
| import React, { Component } from 'react'; | ||
| import { DragSource, DropTarget } from 'react-dnd'; | ||
| import { Checkbox, IconReorder24, spacersNum } from '@dhis2/ui'; | ||
| import TableCell from '@material-ui/core/TableCell'; | ||
| import { withStyles } from '@material-ui/core/styles'; | ||
| import React, { useRef } from 'react'; | ||
| import { useDrag, useDrop } from 'react-dnd'; | ||
| import { DataTableRow, DataTableCell, Checkbox } from '@dhis2/ui'; | ||
|
|
||
| const styles = () => ({ | ||
| checkbox: { | ||
| marginTop: spacersNum.dp12, | ||
| marginBottom: spacersNum.dp12, | ||
| }, | ||
| }); | ||
| const ItemTypes = { | ||
| LISTITEM: 'listItem', | ||
| }; | ||
|
|
||
| type Props = { | ||
| id: string, | ||
| visible: boolean, | ||
| text: string, | ||
| handleToggle: (id: string) => void, | ||
| isDragging: () => void, | ||
| connectDragSource: (any) => void, | ||
| connectDropTarget: (any) => void, | ||
| classes: { | ||
| checkbox: string, | ||
| } | ||
| }; | ||
|
|
||
| const style = { | ||
| cursor: 'move', | ||
| outline: 'none', | ||
| }; | ||
|
|
||
| const ItemTypes = { | ||
| LISTITEM: 'listItem', | ||
| index: number, | ||
| handleToggle: (id: string) => any, | ||
| moveListItem: (dragIndex: number, hoverIndex: number) => void, | ||
| }; | ||
|
|
||
| const cardSource = { | ||
| beginDrag(props) { | ||
| return { | ||
| id: props.id, | ||
| index: props.index, | ||
| }; | ||
| }, | ||
| }; | ||
| export const DragDropListItem = ({ id, index, text, visible, handleToggle, moveListItem }: Props) => { | ||
| const ref = useRef(null); | ||
|
|
||
| const cardTarget = { | ||
| hover(props, monitor) { | ||
| const dragIndex = monitor.getItem().index; | ||
| const hoverIndex = props.index; | ||
| const [, drop] = useDrop({ | ||
| accept: ItemTypes.LISTITEM, | ||
| hover(item: { id: string, index: number }) { | ||
| const dragIndex = item.index; | ||
| const hoverIndex = index; | ||
|
|
||
| // Don't replace items with themselves. | ||
| if (dragIndex === hoverIndex) { | ||
| return; | ||
| } | ||
| // Don't replace items with themselves. | ||
| if (dragIndex === hoverIndex) { | ||
| return; | ||
| } | ||
|
|
||
| // Time to actually perform the action. | ||
| props.moveListItem(dragIndex, hoverIndex); | ||
| // Time to actually perform the action | ||
| moveListItem(dragIndex, hoverIndex); | ||
|
|
||
| // Note: we're mutating the monitor item here! | ||
| // Generally it's better to avoid mutations, | ||
| // but it's good here for the sake of performance | ||
| // to avoid expensive index searches. | ||
| monitor.getItem().index = hoverIndex; | ||
| }, | ||
| }; | ||
| // Note: we're mutating the item here! | ||
| // Generally it's better to avoid mutations, | ||
| // but it's good here for the sake of performance | ||
| // to avoid expensive index searches. | ||
| item.index = hoverIndex; | ||
| }, | ||
| }); | ||
|
|
||
| class Index extends Component<Props> { | ||
| render() { | ||
| const { text, isDragging, connectDragSource, connectDropTarget } = this.props; | ||
| const opacity = isDragging ? 0 : 1; | ||
| const [{ isDragging }, drag] = useDrag({ | ||
| type: ItemTypes.LISTITEM, | ||
| item: { id, index }, | ||
| collect: monitor => ({ | ||
| isDragging: monitor.isDragging(), | ||
| }), | ||
| }); | ||
|
|
||
| // $FlowFixMe[incompatible-extend] automated comment | ||
| return connectDropTarget(connectDragSource( | ||
| <tr key={this.props.id} tabIndex={-1} style={{ ...style, opacity }}> | ||
| <TableCell component="th" scope="row"> | ||
| <Checkbox | ||
| checked={this.props.visible} | ||
| tabIndex={-1} | ||
| onChange={this.props.handleToggle(this.props.id)} | ||
| label={text} | ||
| className={this.props.classes.checkbox} | ||
| valid | ||
| dense | ||
| /> | ||
| </TableCell> | ||
| <TableCell> | ||
| <span style={{ float: 'right' }}> | ||
| <IconReorder24 /> | ||
| </span> | ||
| </TableCell> | ||
| </tr>, | ||
| )); | ||
| } | ||
| } | ||
| drag(drop(ref)); | ||
|
|
||
| export const DragDropListItemPlain = | ||
| DragSource( | ||
| ItemTypes.LISTITEM, | ||
| cardSource, | ||
| (connect, monitor) => ({ connectDragSource: connect.dragSource(), isDragging: monitor.isDragging() }), | ||
| )(DropTarget( | ||
| ItemTypes.LISTITEM, | ||
| cardTarget, | ||
| connect => ({ connectDropTarget: connect.dropTarget() }), | ||
| )(Index)); | ||
| const opacity = isDragging ? 0.5 : 1; | ||
|
|
||
| export const DragDropListItem = withStyles(styles)(DragDropListItemPlain); | ||
| return ( | ||
| <DataTableRow ref={ref} style={{ opacity }} draggable> | ||
| <DataTableCell>{text}</DataTableCell> | ||
| <DataTableCell> | ||
| <Checkbox | ||
| checked={visible} | ||
| onChange={handleToggle(id)} | ||
| valid | ||
| dense | ||
| /> | ||
| </DataTableCell> | ||
| </DataTableRow> | ||
| ); | ||
| }; | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
How about setting the opacity to
0when dragging? This way the drawn row will look empty and can help with the confusion of the hover effect (this is also how it currently works in play with the material UI components).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.
@simonadomnisoru
I agree that setting the opacity to
0makes it better so I will change it to:const opacity = isDragging ? 0 : 1;However, the table row hover is still confusing. I added a picture where I am dragging the Gender, but the hover is marking Last name.
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 use Firefox where the hover issue doesn't happen, but I was able to reproduce it in Chrome. I bealive you can remove the hover styles altogether with something like this in the
DragDropListItem.component.js.// the styles
// and in the render use the class
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.
Thank you, @simonadomnisoru! That works. With the changes, the hover was completely removed, so in addition to your suggested changes, I added a state to tell if any element in the list is being dragged. Based on that state, I will disable the hover. What do you think? I appreciate your help!
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 looks great now 👏