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

Outline and Block appearances for nimble-text-field #345

Merged
merged 27 commits into from
Feb 14, 2022
Merged

Conversation

m-akinc
Copy link
Contributor

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

Pull Request

🤨 Rationale

The design spec for Nimble's text field defines Underline, Outline, and Block appearances. We had implemented only the Underline appearance. This change introduces an appearance attribute on the nimble-text-field and implements the two missing appearances.

👩‍💻 Implementation

We follow the example of the Button and its different appearance modes. We conditionally apply different css based on the appearance.

The design doc does not specify what the control should look like in readonly mode, for any of the three appearances. The implementation we already had (for Underline mode) just removed the underline, leaving just the text content. I styled the other two appearances the same way, as it seemed to be the most consistent approach. It also makes sense to me that a read-only text field is essentially just a stand-alone label (without any other decorative elements).

I also found and fixed a couple existing styling issues, e.g. dimming the control label in disabled mode.

🧪 Testing

In Storybook, manually examined each appearance (including hovering and focusing) with combinations of disabled, readonly, and invalid flags.

✅ Checklist

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

@m-akinc m-akinc marked this pull request as ready for review February 9, 2022 22:04
@mollykreis
Copy link
Contributor

The read-only 'placeholder' text fields accept tab focus with no visual indication of the focus.

It looks like an existing bug, but if it's a simple fix, you could consider addressing it as part of this PR. I can create an issue for it if you think it's outside of the scope of this change.

Copy link
Contributor

@fredvisser fredvisser left a comment

Choose a reason for hiding this comment

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

Looks good to me -👍

Comment on lines 65 to 68
:host(:not([disabled])) .root:hover {
--ni-private-bottom-border-width: var(
--ni-private-hover-bottom-border-width
);
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,

ie

.root:hover {
          --ni-private-bottom-border-width: var(
            --ni-private-hover-bottom-border-width
        );
}

/**/

:host([disabled]) .root:hover {
  --ni-private-bottom-border-width: 1px; /* override this property for the specific state */
}

The reason is to make it more of a look-up table of the component state to the matching selector even if it means more property assignments are made (not a big deal because the majority of values should be tokenized or derived from tokens). See more discussion in: #336 (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.

Made suggested change.

packages/nimble-components/src/text-field/styles.ts Outdated Show resolved Hide resolved
@m-akinc m-akinc merged commit 474dba4 into main Feb 14, 2022
@m-akinc m-akinc deleted the text-appearances branch February 14, 2022 21:26
@rajsite rajsite mentioned this pull request Feb 16, 2022
1 task
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.

5 participants