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

fix/ O3-2253:Marking a field as readonly should render the field control as opposed to switching to view mode #114

Conversation

arodidev
Copy link
Contributor

@arodidev arodidev commented Aug 23, 2023

Requirements

  • This PR has a title that briefly describes the work done including the ticket number. If there is a ticket, make sure your PR title includes a conventional commit label. See existing PR titles for inspiration.
  • My work conforms to the OpenMRS 3.0 Styleguide and design documentation.
  • My work includes tests or is validated by existing tests.

Summary

In "edit" and "enter" mode, the fields marked as read-only should render the controlled in readOnly mode and not the default view mode display

Screenshots

Screenshot 2023-08-23 at 15 50 40

Related Issue

Link to community ticket: https://issues.openmrs.org/browse/O3-2303

Other

@arodidev arodidev changed the title feat/Marking a field as readonly should render the field control as opposed to switching to view mode fix/Marking a field as readonly should render the field control as opposed to switching to view mode Aug 23, 2023
@arodidev arodidev changed the title fix/Marking a field as readonly should render the field control as opposed to switching to view mode fix/ O3-2253:Marking a field as readonly should render the field control as opposed to switching to view mode Aug 23, 2023
@arodidev
Copy link
Contributor Author

@samuelmale @ibacher there is an issue with the carbon radio component ignoring the readOnly attribute passed to it, perhaps it has to do with the carbon version...

@ibacher
Copy link
Member

ibacher commented Aug 24, 2023

@arodidev That's nothing to do with Carbon. The underlying HTML input element doesn't support a readonly attribute for all input types. It should be defined (probably in the ticket) what the behaviour for those fields should be.

@samuelmale
Copy link
Member

@ibacher I agree there is no native support for readonly for the radio element but the latest version of Carbon somehow supports this. I guess it's a CCS tweak in the radio group.

Copy link
Member

@samuelmale samuelmale left a comment

Choose a reason for hiding this comment

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

LGTM

@samuelmale samuelmale merged commit 294c8ad into openmrs:main Aug 24, 2023
7 checks passed
@ibacher
Copy link
Member

ibacher commented Aug 24, 2023

@samuelmale Yeah, I missed that when I looked at Storybook earlier. The readOnly attribute can be applied to the RadioButtonGroup component, not the RadioButton component, although there, they just block onChange() from doing anything and apply some styles.

@samuelmale
Copy link
Member

samuelmale commented Aug 24, 2023

The readOnly attribute can be applied to the RadioButtonGroup component, not the RadioButton component

@ibacher you're correct, but it doesn't work with our supported Carbon version.

@ibacher
Copy link
Member

ibacher commented Aug 24, 2023

@samuelmale True... but we could always just add the "do nothing" on change handler if the field is set to read-only right?

@samuelmale
Copy link
Member

@ibacher that's a brilliant idea but assuming the radio buttons remain interactive, wouldn't this confuse the users?

@ibacher
Copy link
Member

ibacher commented Aug 24, 2023

That's true. We could also add the same Sass rule that gets applied:

  @use "@carbon/colors";
  .cds--radio-button-group--readonly
    .cds--radio-button
    + .cds-radio-button__label
    .cds--radio-button__appearance {
    border-color: colors.$icon-disabled;
  }

  .cds--radio-button-group--readonly .#{$prefix}--radio-button__label {
    cursor: default;
  }

  .cds--radio-button-group--readonly
    .cds--radio-button__label-text {
    cursor: text;
    user-select: text;
  }

Then just add the .cds--radio-button-group--readonly to RadioButtonGroup. The BEM class structure and entity layout is the same, just the rules aren't present yet.

The important thing there is it sets the border-color to disabled while leaving the text alone.

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

Successfully merging this pull request may close these issues.

3 participants