Skip to content

Conversation

@alisonailea
Copy link
Contributor

@alisonailea alisonailea commented Feb 16, 2024

Related Issue: #7180

Summary

Adds the following component tokens to the calcite-input component styles

--calcite-input-corner-radius: defines the border radius of the component.
--calcite-input-text-color: defines the text color of the component.
--calcite-input-border-color: defines the border color of the component.
--calcite-input-background-color: defines the background color of the component.
--calcite-input-button-background-color: defines the background color of a button element in the component.
--calcite-input-button-background-color-hover: defines the background color of a :hover-ed button element in the component.
--calcite-input-button-background-color-active: defines the background color of an :active button element in the component.
--calcite-input-icon-color: defines the color of an icon element in the component.
--calcite-input-button-icon-color-hover: defines the color of an icon element when it's parent is hovered in the component.
--calcite-input-prefix-text-color: defines the prefix text color in the component.
--calcite-input-prefix-background-color: defines the prefix background color in the component.
--calcite-input-suffix-text-color: defines the suffix text color in the component.
--calcite-input-suffix-background-color: defines the suffix background color in the component.
--calcite-input-placeholder-text-color: defines the color of placeholder text in the component.
--calcite-input-shadow: defines the box-shadow of the component.

@alisonailea alisonailea changed the base branch from main to epic/7180-component-tokens February 16, 2024 03:04
* @prop --calcite-input-button-background-color-active: defines the background color of an :active button element in the component.
* @prop --calcite-input-icon-color: defines the color of an icon element in the component.
* @prop --calcite-input-icon-color-hover: defines the color of an icon element when it's parent is hovered in the component.
* @prop --calcite-input-prefixsuffix-text-color: defines the text color of a prefix/suffix element in the component.
Copy link
Contributor

Choose a reason for hiding this comment

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

I could see users wanting to customize these individually pretty easily, wdyt cc @SkyeSeitz @ashetland

* @prop --calcite-input-button-background-color-hover: defines the background color of a :hover-ed button element in the component.
* @prop --calcite-input-button-background-color-active: defines the background color of an :active button element in the component.
* @prop --calcite-input-icon-color: defines the color of an icon element in the component.
* @prop --calcite-input-icon-color-hover: defines the color of an icon element when it's parent is hovered in the component.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can the user set this with calcite-input: hover or is it for a different icon besides the one from ‘icon’ prop?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's different.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe input-button-icon-color and input-button-icon-color-hover just to be clearer? From the name / description it just sounds like the hover color for the icon set above (which can be affected with a parent-level pseudo state override)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

input-icon-color is used for static icons as well. I'll keep this name but update the input-button-icon-color-hover to be clearer.

* These properties can be overridden using the component's tag as selector.
*
* @prop --calcite-input-corner-radius: defines the border radius of the component.
* @prop --calcite-input-text-color: defines the text color of the component.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add an input-shadow that defaults to none?

*
* These properties can be overridden using the component's tag as selector.
*
* @prop --calcite-input-corner-radius: defines the border radius of the component.
Copy link
Contributor

Choose a reason for hiding this comment

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

Conditionally handle the closable button with this value:
Screenshot 2024-02-16 at 9 15 36 AM

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 already have the styles there to prevent an example like you've shown above. The input will not add a rounded border when a ::slotted action is added. However, I can not style the button as shown above as the button is not currently styled in such a way as to allow that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed, we shouldn't apply it to slotted content. But that clear button is rendered internally, IMO we should treat it like the number buttons which you already handle:

border-start-start-radius: var(--calcite-input-corner-radius);
border-end-start-radius: var(--calcite-input-corner-radius);

It's only an issue when the close button is adjacent to the edge, you're already handling it in these cases, I think it just needs a conditional for the above state:
Screenshot 2024-02-16 at 5 23 32 PM
Screenshot 2024-02-16 at 5 22 30 PM

Copy link
Contributor

@macandcheese macandcheese left a comment

Choose a reason for hiding this comment

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

Looking really good! Two main things:

  • Handle the clearable case when its the last rendered element.
  • I think there are some extra border colors being set on focus / invalid.

Previously:
Screenshot 2024-02-16 at 5 40 20 PM
Screenshot 2024-02-16 at 5 40 34 PM
Screenshot 2024-02-16 at 5 40 41 PM

With this PR:
Screenshot 2024-02-16 at 5 42 12 PM
Screenshot 2024-02-16 at 5 42 14 PM
Screenshot 2024-02-16 at 5 42 30 PM

I don't mind the invalid styling around the suffix / prefix, but the focus color around number spinners shouldn't be applied I don't think.

@macandcheese
Copy link
Contributor

macandcheese commented Feb 21, 2024

Colors look good! Can we fire off a Chromatic build for this?

Border radius looks good on close button in some states :
Screenshot 2024-02-20 at 6 00 35 PM
Screenshot 2024-02-20 at 5 57 32 PM

But I think it might need another pass for certain states :

Screenshot 2024-02-20 at 5 58 02 PM Screenshot 2024-02-20 at 5 59 17 PM Screenshot 2024-02-20 at 5 59 21 PM

@alisonailea
Copy link
Contributor Author

@macandcheese Thanks for the catch! I did find one error but I also updated the storybook story to remove empty "prefix-text" and "suffix-text" which were causing some of the unexpected layout bugs you saw.

Latest screenshots

Screenshot 2024-02-20 at 10 13 26 PM Screenshot 2024-02-20 at 10 13 35 PM Screenshot 2024-02-20 at 11 01 51 PM Screenshot 2024-02-20 at 11 02 09 PM Screenshot 2024-02-20 at 11 02 30 PM Screenshot 2024-02-20 at 11 02 46 PM Screenshot 2024-02-20 at 11 03 37 PM Screenshot 2024-02-20 at 11 04 12 PM Screenshot 2024-02-20 at 11 04 23 PM Screenshot 2024-02-20 at 11 04 29 PM Screenshot 2024-02-20 at 11 04 38 PM

@alisonailea alisonailea added the pr ready for visual snapshots Adding this label will run visual snapshot testing. label Feb 21, 2024
Copy link
Contributor

@macandcheese macandcheese left a comment

Choose a reason for hiding this comment

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

🏅

@alisonailea alisonailea merged commit a5949cd into epic/7180-component-tokens Feb 21, 2024
@alisonailea alisonailea deleted the astump/7180-input branch February 21, 2024 21:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr ready for visual snapshots Adding this label will run visual snapshot testing.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants