-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Set custom cursor theme when RadioButton and CheckBox are disabled #6867
Set custom cursor theme when RadioButton and CheckBox are disabled #6867
Conversation
Hey @LegerG sorry for the delay reviewing this, hoping to get a chance to take a closer look at this early next week. Thanks for your patience! |
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'm thinking that we could handle setting the cursor style with a global.input.disabled.cursor
prop in the theme instead of needing checkbox.disabled
or radioButton.disabled
. This would keep the theme structure a little simpler as well as allow for setting the stying of the disabled cursor across all input components.
d947035
to
98b9eb7
Compare
Thank you for your response, I have updated the PR. |
Hey @LegerG thanks for updating the PR! I'm thinking support should be added for |
Hello, I have almost done the code but I have some points I need to discuss with you
Different cursor inside the component SelectMultiple issue when disabled If you need more details about one of these points, don't hesitate. :) |
Hey there, thanks for your contribution so far and your willingness to incorporate feedback! After a bit more discussion within our team, given your comment, we've decided it would be best to support this from the I imagine in With this direction, Anchor and Button would also receive this treatment and so would any inputs that support Thank you for your notes about RangeSelector/DateInput For the DefaultSelectTextInput in the Select components I think if |
I have pushed for a new version following your requirements. It is big PR, so there is a list of all components impacted in the source code (for the others, it is only tests or stories):
For now, I haven't written any tests (except for the RadioButton and CheckBox components). I plan to do it in the next commit. Do I need to write tests for all components affected by the update, or only those whose source code has been modified? (for example, only for RadioButton and not for RadioButtonGroup). Don't hesitate if you need more details 😃 |
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.
Just a few comments, coming along nicely!
c0adae7
to
dacd001
Compare
Hello, I have applied all your feedbacks, thanks for the review. |
import { Grommet, Button, Box } from '../..'; | ||
import { buttonKindTheme } from './theme/buttonKindTheme'; | ||
|
||
describe('Button kind', () => { |
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 don't think this file should be deleted.
@@ -552,7 552,7 @@ describe('DataTable', () => { | |||
} | |||
return ( | |||
<Box> | |||
{row.a} : {row.b}{' '} | |||
{row.a} :{row.b} |
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 don't think this file should be changed by this PR, looks unrelated/potentially a local linting change
@@ -55,6 55,8 @@ const Label = styled(Text)` | |||
props.theme.fileInput && | |||
props.theme.fileInput.label && | |||
props.theme.fileInput.label.extend}; | |||
cursor: ${(props) => |
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.
Similar comment to above, but this should appear before the extend. Also, instead of adding default
as a fallback, could we simplify the code in all instances as:
props.disabled && props.theme.global.controls.disabled.cursor ? `cursor: ${props.theme.global.controls.disabled.cursor};` : '';
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.
This should simplify the CSS to not add cursor: default in as many places.
import { Box, FileInput, Grommet } from 'grommet'; | ||
|
||
export const Disabled = () => ( | ||
<Grommet |
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 has a custom theme, can you place it under a CustomThemed folder like your Anchor story?
@@ -1,27 1,41 @@ | |||
import React from 'react'; | |||
import React, { useState } from 'react'; |
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 the "Disabled" story already captures this behavior, can we leave this story untouched?
@@ -202,7 202,7 @@ exports[`Grommet hpe theme 1`] = ` | |||
font-size: 18px; | |||
line-height: 24px; | |||
background-color: #FFFFFF; | |||
color: #555555; | |||
color: #6F6F6F; |
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 like this is out of date with master.
|
||
return ( | ||
<Grommet | ||
theme={{ global: { control: { disabled: { cursor: 'not-allowed' } } } }} |
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.
Same comment as above about putting this in a CustomThemed folder.
disabled | ||
/> | ||
</Box> | ||
<Grommet |
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 think there are enough stories demoing this behavior, can we leave this story as it was?
{...props} | ||
/> | ||
</Box> | ||
<Grommet |
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 think there are enough stories demoing this behavior, can we leave this story as it was?
const onChange = (event) => setValue(event.target.value); | ||
|
||
return ( | ||
<Grommet |
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 think there are enough stories demoing this behavior, can we leave this story as it was?
@@ -5905,6 5909,8 @@ exports[`Select keyboard navigation timeout 1`] = ` | |||
border-radius: 4px; | |||
outline: none; | |||
border: none; | |||
opacity: 0.3; |
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 seems suspicious that this styling is being added here even though nothing changed in select-test.
Closing due to inactivity, feel free to re-open if this feature is still desired |
What does this PR do?
When a CheckBox or a RadioButton is disabled, it's possible to set a custom cursor, applied on the container components (the cursor is also applied on the label text).
Where should the reviewer start?
src/js/components/CheckBox/StyledCheckBox.js
src/js/components/RadioButton/StyledRadioButton.js
What testing has been done on this PR?
Manual and automated tests
How should this be manually tested?
Override the default theme with
<Grommet theme={{ checkBox: { disabled: { cursor: "not-allowed" } }, radioButton: { disabled: { cursor: "not-allowed" } } }}>
Do Jest tests follow these best practices?
screen
is used for querying.userEvent
is used in place offireEvent
.asFragment()
is used for snapshot testing.Any background context you want to provide?
What are the relevant issues?
#6862
Screenshots (if appropriate)
Do the grommet docs need to be updated?
Yes
Should this PR be mentioned in the release notes?
Yes
Is this change backwards compatible or is it a breaking change?
No