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

Update readonly text field style #359

Merged
merged 4 commits into from
Feb 17, 2022

Conversation

m-akinc
Copy link
Contributor

@m-akinc m-akinc commented Feb 15, 2022

Pull Request

🀨 Rationale

Brandon changed a few things in the text field design, most notably the readonly style.

πŸ‘©β€πŸ’» Implementation

  1. All three appearance modes now have an outline (and no background shading) when readonly:
    image
  2. Block appearance, when disabled, no longer looks like underline. It is just a more dimmed version of the normal block styling.
  3. The background shading for block appearance is slightly darker.
  4. Readonly mode (for all appearances) now uses an I-beam cursor (rather than replacing it with the standard cursor)
  5. I reordered the CSS to follow our grouping/ordering guidelines.

πŸ§ͺ Testing

Manual testing in Storybook

βœ… Checklist

  • I have updated the project documentation to reflect my changes or determined no changes are needed.

Copy link
Contributor

@jattasNI jattasNI left a comment

Choose a reason for hiding this comment

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

Friendly reminder to review the UI status checks and approve them if they look good.

Looks good to me but I also added @mollykreis as a reviewer since my CSS knowledge is still a work in progress and she left good feedback on #345. Let's wait for an approval from her or @rajsite before merging.

}

:host(.invalid) .root {
border-bottom-color: ${failColor};
}

:host([readonly]:not([disabled])) .root {
Copy link
Member

Choose a reason for hiding this comment

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

Instead of using :not can we rely on the cascade, see #345 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought this scenario made sense for using :not (our guidelines seem to allow for using :not sometimes). Consider these designs:
image
All three appearances have the same readonly style, but their disabled styles are slight transformations of their default styles. When both readonly and disabled, we want the disabled style to take precedence. It seems that a simple way to achieve this is to use [readonly]:not([disabled]) in the selector. Otherwise, for each style, I have to explicitly style the [readonly][disabled] case, making sure to overwrite all the settings that were specified in the [readonly] case. That seemed messier to me, but if you disagree, I'll make the change.

Copy link
Member

@rajsite rajsite Feb 17, 2022

Choose a reason for hiding this comment

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

That seemed messier to me, but if you disagree, I'll make the change.
Otherwise, for each style, I have to explicitly style the [readonly][disabled] case

Can you make a quick branch to compare them? I don't think I understand why this would be so "messy".

If the squad agrees that we prefer to mix explicit states with :not logic then lets do what folks think is more maintainable. We should come up with guidelines so we can review it consistently.

I'm going to push hard against seeing things like element:not(blah1):not(blah2):not(balh3) because that becomes unmaintainable. Folks making patches tend to avoid disentangling chained not selectors and instead add more to the chain. I'm essentially advocating for never entangling them in the first place.

If we look at FAST's usages the vast majority seem to be for the disabled state as well. Would could say that disabled state is an exception and it's okay to sprinkle every other line with :not([disabled])

I think for the disabled state in specific, the reason FAST goes through the hoops for saying all this stuff does not apply to the disabled state is because there is not an easy way to trump all the selectors. Ideally we just want to put disabled state at the end and have it override all the intermediate states. The layers proposal will help with this: https://www.youtube.com/watch?v=ilrPpSQJb3U. And I think migrating to layers will be easier with explicit states.

Copy link
Member

Choose a reason for hiding this comment

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

Approved as we don't need to necessarily block the PR on this discussion

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will submit like this and try it without :not so we can compare.

Copy link
Member

Choose a reason for hiding this comment

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

sounds good πŸ‘

@m-akinc m-akinc merged commit 0663feb into main Feb 17, 2022
@m-akinc m-akinc deleted the users/makinc/readonly-text-field-style branch February 17, 2022 17:24
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