Set custom cursor theme when RadioButton and CheckBox are disabled #6867

LegerG · 2023-07-08T18:25:11Z

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?

[ x] screen is used for querying.
[ x] The correct query is used. (Refer to this list of queries)
[ x] userEvent is used in place of fireEvent.
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

jcfilben · 2023-07-14T03:47:49Z

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!

jcfilben

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.

LegerG · 2023-07-21T07:02:14Z

Thank you for your response, I have updated the PR.
I will update other inputs in the next PR.

jcfilben · 2023-07-26T22:39:52Z

Hey @LegerG thanks for updating the PR! I'm thinking support should be added for global.input.disabled.cursor to all the input components within one PR. I know you mentioned tackling the rest of this work in a subsequent PR but I think ideally we don't want to have a gap of time where we have added this global theme prop but it is only working for some of the input components.

LegerG · 2023-08-29T06:12:56Z

Hello, I have almost done the code but I have some points I need to discuss with you

RangeSelector and DateInput don't have disabled props.
Anchor and Button are not considered as inputs in the documentation but they are included in some components like SelectMultiple or FileInput. So there are some parts of the component where the cursor is different (see the screen below).
MaskedInput documentation misses the disabled props, but I have implemented the feature anyway, because the props are in the code.
DefaultSelectTextInput in the Select components has a comment about the disabled props. I have not implemented the feature for this one. I don't know what is the best to do.(https://github.com/grommet/grommet/blob/master/src/js/components/Select/DefaultSelectTextInput.js#L7)
SelectMultiple has a bug with the showSelectedInline props and disabled. The showed selected items are not disabled. I have not fixed this feature

Different cursor inside the component

SelectMultiple issue when disabled

If you need more details about one of these points, don't hesitate. :)

taysea · 2023-09-14T19:57:34Z

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 theme.global.control.disabled.cursor object instead of the previous discussion around theme.global.input.

I imagine in disabledStyle, the cursor line could look something like theme.global.control.disabled.cursor || 'default'. Would you be willing to make that adjustment?

With this direction, Anchor and Button would also receive this treatment and so would any inputs that support disabled props. Regarding documentation of disabled prop on grommet docs, our approach has been not to document properties that are native HTML properties.

Thank you for your notes about RangeSelector/DateInput disabled support -- we'll file separate tickets to look into those. And I already filed one for the SelectMultiple issue you mentioned. We can take care of those issues separate from this PR.

For the DefaultSelectTextInput in the Select components I think if theme.global.control.disabled.cursor is defined we can use that for the cursor styling, otherwise we can rely on what is already there (defaultCursor={disabled === true || undefined})

LegerG · 2023-10-01T17:19:31Z

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):

Anchor
Button
CheckBox
FileInput
RadioButton
RangeInput
Select

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 😃

jcfilben

Just a few comments, coming along nicely!

src/js/utils/styles.js

src/js/themes/base.js

src/js/components/Anchor/StyledAnchor.js

src/js/components/Select/stories/Disabled.js

src/js/components/Select/StyledSelect.js

src/js/components/Anchor/stories/Disabled.js

src/js/components/Button/StyledButton.js

src/js/components/Button/stories/Disabled.js

src/js/components/CheckBox/stories/Simple.js

src/js/components/CheckBoxGroup/stories/Disabled.js

src/js/components/FileInput/StyledFileInput.js

LegerG · 2024-01-26T14:46:15Z

Hello, I have applied all your feedbacks, thanks for the review.

src/js/components/Button/StyledButton.js

taysea · 2024-02-01T21:03:40Z

src/js/components/Button/__tests__/Button-kind-test.js

-import { Grommet, Button, Box } from '../..';
-import { buttonKindTheme } from './theme/buttonKindTheme';
-
-describe('Button kind', () => {


I don't think this file should be deleted.

taysea · 2024-02-01T21:05:47Z

src/js/components/DataTable/__tests__/DataTable-test.tsx

@@ -552,7 +552,7 @@ describe('DataTable', () => {
            }
            return (
              <Box>
-                {row.a} : {row.b}{' '}
+                {row.a} :{row.b}


I don't think this file should be changed by this PR, looks unrelated/potentially a local linting change

taysea · 2024-02-01T21:08:13Z

src/js/components/FileInput/FileInput.js

@@ -55,6 +55,8 @@ const Label = styled(Text)`
    props.theme.fileInput &&
    props.theme.fileInput.label &&
    props.theme.fileInput.label.extend};
+  cursor: ${(props) =>


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};` : '';

This should simplify the CSS to not add cursor: default in as many places.

taysea · 2024-02-01T21:09:47Z

src/js/components/FileInput/stories/Disabled.js

+import { Box, FileInput, Grommet } from 'grommet';
+
+export const Disabled = () => (
+  <Grommet


Since this has a custom theme, can you place it under a CustomThemed folder like your Anchor story?

taysea · 2024-02-01T21:10:21Z

src/js/components/FileInput/stories/Multiple.js

@@ -1,27 +1,41 @@
-import React from 'react';
+import React, { useState } from 'react';


Since the "Disabled" story already captures this behavior, can we leave this story untouched?

taysea · 2024-02-01T21:11:12Z

src/js/components/Grommet/__tests__/__snapshots__/Grommet-test.js.snap

@@ -202,7 +202,7 @@ exports[`Grommet hpe theme 1`] = `
  font-size: 18px;
  line-height: 24px;
  background-color: #FFFFFF;
-  color: #555555;
+  color: #6F6F6F;


Looks like this is out of date with master.

taysea · 2024-02-01T21:11:44Z

src/js/components/MaskedInput/stories/Disabled.js

+
+  return (
+    <Grommet
+      theme={{ global: { control: { disabled: { cursor: 'not-allowed' } } } }}


Same comment as above about putting this in a CustomThemed folder.

taysea · 2024-02-01T21:12:42Z

src/js/components/RadioButton/stories/Disabled.js

-      disabled
-    />
-  </Box>
+  <Grommet


I think there are enough stories demoing this behavior, can we leave this story as it was?

taysea · 2024-02-01T21:12:55Z

src/js/components/RadioButtonGroup/stories/Disabled.js

-        {...props}
-      />
-    </Box>
+    <Grommet


I think there are enough stories demoing this behavior, can we leave this story as it was?

taysea · 2024-02-01T21:13:11Z

src/js/components/RangeInput/stories/Disabled.js

+  const onChange = (event) => setValue(event.target.value);
+
+  return (
+    <Grommet


I think there are enough stories demoing this behavior, can we leave this story as it was?

taysea · 2024-02-01T21:19:07Z

src/js/components/Select/__tests__/__snapshots__/Select-test.js.snap

@@ -5905,6 +5909,8 @@ exports[`Select keyboard navigation timeout 1`] = `
  border-radius: 4px;
  outline: none;
  border: none;
+  opacity: 0.3;


It seems suspicious that this styling is being added here even though nothing changed in select-test.

jcfilben added the PRty Biweekly PR reviews by grommet team with hoping of closing such PRs label Jul 20, 2023

jcfilben reviewed Jul 20, 2023

View reviewed changes

jcfilben removed the PRty Biweekly PR reviews by grommet team with hoping of closing such PRs label Jul 20, 2023

LegerG force-pushed the feat/custom-disabled-cursor-theme branch from d947035 to 98b9eb7 Compare July 21, 2023 06:59

jcfilben added the waiting Awaiting response to latest comments label Aug 2, 2023

jcfilben added PRty Biweekly PR reviews by grommet team with hoping of closing such PRs and removed waiting Awaiting response to latest comments labels Aug 31, 2023

taysea added waiting Awaiting response to latest comments and removed PRty Biweekly PR reviews by grommet team with hoping of closing such PRs labels Sep 14, 2023

jcfilben removed the waiting Awaiting response to latest comments label Oct 16, 2023

jcfilben reviewed Oct 23, 2023

View reviewed changes

src/js/utils/styles.js Outdated Show resolved Hide resolved

src/js/themes/base.js Outdated Show resolved Hide resolved

src/js/components/Anchor/StyledAnchor.js Outdated Show resolved Hide resolved

jcfilben added the waiting Awaiting response to latest comments label Oct 25, 2023

jcfilben added PRty Biweekly PR reviews by grommet team with hoping of closing such PRs and removed waiting Awaiting response to latest comments labels Nov 3, 2023