-
Notifications
You must be signed in to change notification settings - Fork 933
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
No Pointer style cursor for table head when no hint or sorting #910
Conversation
With no sorting or a hint, the cursor still shows a pointer. I'm not saying this is the best solution, but I think the header shouldn't shouldn't show a pointer style of cursor when there is nothing to click on or additional behavior from hovering.
@gabrielliwerant Would you be looking to get this change in soon? Is there anything I need to do with this? I imagine you may want to do this change in less of a hacky type fix, but I can make changes to the PR to help get it in faster. |
@lalong13 Just about to look at a few PRs right now. Will keep you updated. |
src/components/TableHeadCell.js
Outdated
@@ -161,7 +165,7 @@ class TableHeadCell extends React.Component { | |||
</span> | |||
</Tooltip> | |||
) : ( | |||
<div className={classes.sortAction}> | |||
<div className={hint ? classes.sortAction : classes.noSortAction}> |
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.
What if we just do hint ? classes.sortAction : null
? Then we don't even need replacement styles and it looks like it prevents the pointer cursor from showing.
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.
@lalong13 Any interest in submitting that change? If not, I can open up something based on this, just wanted to make sure before using your work or going a different way.
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.
@gabrielliwerant Sorry for the delay. I had assumed that display
and verticalAlign
style properties were still desired. I don't mind not giving it a class name personally. It just might be weird if the other stylings change.
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 appreciate the cautious approach, but those styles are only needed when the div acts as a container for other things, like sort and hint. They serve no useful function when we don't have a sort or hint.
If some other thing is added here in the future, we can add styles back, but for now, I think it's cleaner to just remove them.
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 made the suggested change. I can roll the commits up as well if needed/desired.
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.
Excellent, will verify soon! What do you mean by "roll the commits up?"
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.
Make the two commits into just one commit.
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.
Oh, gotcha. No need, I squash them when I merge.
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.
@gabrielliwerant Do I need to do anything else for this at this point?
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.
Not sure, I need to re-checkout and look everything over, just a bit backed up with PRs and such, but I'll swing around to it before the next minor release.
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.
Looks good, thanks for making that change!
…/activeIcon-for-search-not-reset-on-toggle * 'master' of github.com:gregnb/mui-datatables: 2.13.0 Prettify files Feature/issue 1036 fixed header (gregnb#1071) Update webpack dev server and related dependencies (gregnb#1069) Feature/issue 871 (setCellHeaderProps) and Feature/issue 714 (setTableProps) (gregnb#889) Feature/issue 937 array and custom update filter support (gregnb#1067) Bugfix/issue 1052 csv download issues (gregnb#1064) Hide toggle button when the row is not expandable (gregnb#1025) add test case to verify reset type (gregnb#1006) Update .gitignore and adding yarn.lock (gregnb#1004) Serverside sorting example (gregnb#986) No Pointer style cursor for table head when no hint or sorting (gregnb#910) Added disableToolbarSelect option (gregnb#874) Add table properties option (gregnb#670)
…b#910) * No Pointer style cursor when no hint or sorting With no sorting or a hint, the cursor still shows a pointer. I'm not saying this is the best solution, but I think the header shouldn't shouldn't show a pointer style of cursor when there is nothing to click on or additional behavior from hovering. * Update TableHeadCell.js
With no sorting or a hint, the cursor still shows a pointer. I'm not saying this is the best solution, but I think the header shouldn't shouldn't show a pointer style of cursor when there is nothing to click on or additional behavior from hovering.