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.
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
feat: highlight editable cells on hover #1087
feat: highlight editable cells on hover #1087
Changes from 10 commits
4ce98a4
3f328cc
c7a280c
c3fb6f0
50b9c12
47b5bdc
bd79761
2a8c8c6
d92c2b2
fdec08f
3c13d72
b3658bf
9be0585
a6eec09
257e758
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Is this enum supposed to be actively changing the coloring/styling/behavior of the style, or is it just a dummy enum to show what a select field looks like in the table?
If it's just a dummy enum, can we make the enum values something like
Foo = "Foo"
andBar = "Bar
"` just to highlight their meaningless-ness?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 know this is just a story, but can we rename this to
setRow
, which better describes what it does? Also would prefer afunction
.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.
Since this story is a react component I'll change it to be a
useCallback
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.
@0ces afaict we're not really setting this on "the cell", but instead pushing it down into the child
TextField
's DOM? If so, maybe it shouldn't really be a "table style" and instead each component can know how to style itself has "editable" or "borderless: unless-hovered" or what not.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 could envision eventually needing other non-
TextFieldBase
components controlling the same "I'm hovered, so don't hover the other editable components in the row anymore", so I think it'd potentially be neat to:ROW_CSS_SELECTOR
toBorderHoverParent
textFieldBaseWrapper
and name itBorderHoverChild
To have the names align to the general capability we're adding, which is that should a component have
borderOnHover=true
set, it will hook up its styles to "whatever BorderHoverParent", as long as said BHP doesn't already have a hovered BHC.