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

refactor: [DHIS2-17650] Replace Material-UI Table, TableBody, TableCell, TableHead and TableRow #3721

Conversation

henrikmv
Copy link
Contributor

@henrikmv henrikmv commented Jul 20, 2024

DHIS2-17650

  • Changed from Material-UI Table, TableBody, TableCell, TableHead and TableRow to DHIS2/UI
  • Style changes: Checkboxes are placed to the right to keep them away from the drag/drop icon. Small changes to the table (added border, header row color, etc)
  • Changed from using DragSource and DropTarget to using useDrag and useDrop
  • Had to remove the div to style the table properly. Since DndProvider was expecting the div, I got this error message: Only native element nodes can now be passed to React DnD connectors. You can either wrap DataTableRow into a div or turn it into a drag source or a drop target itself. Solved by using useRef to set the DataTableRow to ref.
  • Updated Cypress tests to look after the checkbox in tablerow

@henrikmv henrikmv marked this pull request as ready for review July 28, 2024 17:18
@henrikmv henrikmv requested a review from a team as a code owner July 28, 2024 17:18
Copy link
Contributor

@simonadomnisoru simonadomnisoru left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link

github-actions bot commented Jul 29, 2024

@eirikhaugstulen eirikhaugstulen added e2e record Apply this label to a pull request to trigger recording of E2E tests on Cypress Cloud testing and removed e2e-tests testing labels Jul 30, 2024
@henrikmv
Copy link
Contributor Author

@eirikhaugstulen requested some changes to improve usability, so I have implemented them now.

Note: I can see that the hover effect can be confusing under drag, but I found it hard to fix it.

Would you give me another review with the new changes implemented @simonadomnisoru? Thanks!

cardTarget,
connect => ({ connectDropTarget: connect.dropTarget() }),
)(Index));
const opacity = isDragging ? 0.5 : 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

Note: I can see that the hover effect can be confusing under drag, but I found it hard to fix it.

How about setting the opacity to 0 when 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).

Copy link
Contributor Author

@henrikmv henrikmv Jul 31, 2024

Choose a reason for hiding this comment

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

Skjermbilde 2024-07-31 kl  11 35 33

@simonadomnisoru
I agree that setting the opacity to 0 makes 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.

Copy link
Contributor

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

const styles = {
    rowWithoutHover: {
        '&:hover > td': {
            backgroundColor: 'transparent !important',
        },
    },
};

// and in the render use the class

return (
        <DataTableRow ref={ref} style={{ opacity }} className={classes.rowWithoutHover} draggable>
            <DataTableCell>{text}</DataTableCell>
            ......

Copy link
Contributor Author

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!

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks great now 👏

@henrikmv henrikmv added e2e-tests and removed e2e record Apply this label to a pull request to trigger recording of E2E tests on Cypress Cloud labels Aug 4, 2024
Copy link
Contributor

@eirikhaugstulen eirikhaugstulen left a comment

Choose a reason for hiding this comment

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

Great work @henrikmv!

Copy link

@geethaalwan geethaalwan left a comment

Choose a reason for hiding this comment

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

Tested successfully on 2.42 version

@henrikmv henrikmv merged commit 09905d7 into master Aug 12, 2024
43 checks passed
@henrikmv henrikmv deleted the hv/feat/DHIS2-17650_ReplaceMaterialUIComponentsInDragAndDropTableColumnSelector branch August 12, 2024 15:42
@dhis2-bot
Copy link
Contributor

🎉 This PR is included in version 100.77.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

5 participants