-
Notifications
You must be signed in to change notification settings - Fork 78
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
feat(input-date-picker): add component tokens #11734
base: dev
Are you sure you want to change the base?
feat(input-date-picker): add component tokens #11734
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is updating 2 different groups of components, so can you split this into separate PRs for consistency and better tracking in #7180?
packages/calcite-components/src/components/input-date-picker/input-date-picker.scss
Outdated
Show resolved
Hide resolved
packages/calcite-components/src/components/input-date-picker/input-date-picker.scss
Show resolved
Hide resolved
packages/calcite-components/src/components/input-date-picker/input-date-picker.scss
Outdated
Show resolved
Hide resolved
packages/calcite-components/src/components/input-date-picker/input-date-picker.scss
Outdated
Show resolved
Hide resolved
packages/calcite-components/src/components/input-date-picker/input-date-picker.scss
Outdated
Show resolved
Hide resolved
packages/calcite-components/src/components/input-date-picker/input-date-picker.scss
Outdated
Show resolved
Hide resolved
packages/calcite-components/src/components/input-date-picker/input-date-picker.scss
Outdated
Show resolved
Hide resolved
packages/calcite-components/src/components/input-date-picker/input-date-picker.scss
Outdated
Show resolved
Hide resolved
Q: Should we add tokens to configure the input part of input-date-picker similar to auto-complete? |
packages/calcite-components/src/components/input-date-picker/input-date-picker.e2e.ts
Outdated
Show resolved
Hide resolved
packages/calcite-components/src/components/input-date-picker/input-date-picker.scss
Outdated
Show resolved
Hide resolved
* @prop --calcite-input-date-picker-icon-color: Specifies the component's icon color. | ||
* @prop --calcite-input-date-picker-icon-color-hover: Specifies the component's icon color on hover. | ||
|
||
* @prop --calcite-input-date-picker-input-background-color: Specifies the component's input background color. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per the component token guidelines, can we share some parent tokens or is there a use case to allow independent overrides? I think like we can drop the following:
--calcite-input-date-picker-input-background-color
--calcite-input-date-picker-input-border-color
--calcite-input-date-picker-input-icon-color
--calcite-input-date-picker-input-shadow
--calcite-input-date-picker-input-corner-radius
(if we add a parent-level prop)--calcite-input-date-picker-input-text-color
(if we add a parent-level prop)--calcite-input-date-picker-input-placeholder-text-color
(if we add a parent-level prop)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, yeah not sure. I know for the case of a direct child with the same namespace - like Accordion / Accordion Item - we were told to try to put them on the parent until use cases arose for specific child ones (the example at that guideline link you shared uses Combobox / Combobox Item)
But for cases like this, where we are rendering internal elements - I think it's a bad user story to have to know there is a calcite-input
inside. We just saw this with the Panel and its internally rendered Actions. So here I'd think these pass through make sense... just IMO. cc @matgalla for input
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have the same question for fab
with internal button
@matgalla
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there is a bit of confusion regarding shared tokens. AFAIU, shared tokens are applicable if the visual pattern is similar. It may not be clear to the user if calcite-input
is being used for calcite-input-date-picker
given how it is configured visually with chevron & divider for range. In contrary, it is easy to understand if calcite-chip
is associated with any component.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good points all around. I agree that it hurts the user story to have to know that calcite-input
is being used. This case might be more obvious that it's an input, but I'm sure that we'll have other instances where it isn't as clear.
Question 1 - If a user changed --calcite-input-background-color
at a higher level, would they expect the date picker to have changed as well? I probably would, but I'd want to confirm that with devs actively theming with Calcite today.
Question 2 - Would we be able to add some info directly into the code to clarify which nested components are themable within the component the user is working with? I'm wondering if that could help alleviate the amount of guesswork when changing those variables.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good point ☝️. So far we've been focusing on input
as the internal component and have given no consideration to date-picker
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@matgalla @macandcheese @Elijbet @anveshmekala Can we have a quick discussion on this tomorrow to make sure we're on the same page?
But for cases like this, where we are rendering internal elements - I think it's a bad user story to have to know there is a calcite-input inside. We just saw this with the Panel and its internally rendered Actions.
Agreed. This could be frustrating for users if subcomponent styling changes between releases.
So here I'd think these pass through make sense... just IMO.
Could you clarify what you mean by “these pass through”?
Hm, yeah not sure. I know for the case of a direct child with the same namespace - like Accordion / Accordion Item - we were told to try to put them on the parent until use cases arose for specific child ones (the accordion at that link uses Combobox / Combobox Item)
&
I think there is a bit of confusion regarding shared tokens. AFAIU, shared tokens are applicable if the visual pattern is similar. It may not be clear to the user if calcite-input is being used for calcite-input-date-picker given how it is configured visually with chevron & divider for range. In contrary, it is easy to understand if calcite-chip is associated with any component.
The following path from this decision tree seems to apply here since the inputs do share a visual pattern:
flowchart TD
add{add a new token. Confirm the naming schema with the Calcite team.}
A1[Looking at a Calcite component's CSS file]
B1[this component uses <a href="https://github.com/Esri/calcite-design-system/wiki/tokens-component#3-review-component-for-tokenizable-styles">tokenizable styles<sup>*</sup></a>]
D1[grouped components/elements share the design pattern/value]
F1[sub-components are slotted]
G2[component needs to override the tokens of the sub-component]
A1 --> B1
B1 -- Yes --> D1
D1 -- Yes --> F1
F1 -- No --> G2
G2 -- Yes --> add
Question 1 - If a user changed --calcite-input-background-color at a higher level, would they expect the date picker to have changed as well? I probably would, but I'd want to confirm that with devs actively theming with Calcite today.
Curious to hear others’ thoughts. Right now it is hard to say without any strong theming examples using component tokens.
Question 2 - Would we be able to add some info directly into the code to clarify which nested components are themable within the component the user is working with? I'm wondering if that could help alleviate the amount of guesswork when changing those variables.
That might help, but probably not enough if the styling surface changes between releases. Developers need a consistent API so underlying changes to components don't cause regressions.
This is a good point ☝️. So far we've been focusing on input as the internal component and have given no consideration to date-picker.
This PR should target input-date-picker
and all its subcomponents.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah let's meet to make sure we cover our bases with all these details
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Summarizing our meeting:
- add sub-component CSS props as needed, focusing on the core styles
- afterward, based on user feedback, we can consider expanding the token set
- leverage parent-level CSS props whenever possible
- side note: consider having the doc show related/inherited props from parent/child components
Worth noting that 1–3 still align with the component design token guidelines. 🎉
@matgalla @macandcheese @Elijbet @anveshmekala LMK if I missed anything.
packages/calcite-components/src/components/input-date-picker/input-date-picker.scss
Show resolved
Hide resolved
packages/calcite-components/src/components/input-date-picker/input-date-picker.scss
Show resolved
Hide resolved
packages/calcite-components/src/components/input-date-picker/input-date-picker.scss
Outdated
Show resolved
Hide resolved
packages/calcite-components/src/custom-theme/input-date-picker.ts
Outdated
Show resolved
Hide resolved
…ving docs, adding e2e tests for more coverage for range and vertical range layouts
Related Issue: #7180
Adds the following tokens to
input-date-picker
component:--calcite-input-date-picker-background-color
: Specifies the component's background color.--calcite-input-date-picker-border-color
: Specifies the component's border color.--calcite-input-date-picker-shadow
: Specifies the component's shadow.--calcite-input-date-picker-divider-color
: Specifies the component's divider color.--calcite-input-date-picker-icon-color
: Specifies the component's icon color.--calcite-input-date-picker-icon-color-hover
: Specifies the component's icon color on hover.--calcite-input-date-picker-input-background-color
: Specifies the component's input background color.--calcite-input-date-picker-input-border-color
: Specifies the component's input border color.--calcite-input-date-picker-input-corner-radius
: Specifies the component's input corner radius.--calcite-input-date-picker-input-shadow
: Specifies the component's input shadow.--calcite-input-date-picker-input-icon-color
: Specifies the component's input icon color.--calcite-input-date-picker-input-icon-color-hover
: Specifies the component's input icon color on hover.--calcite-input-date-picker-input-text-color
: Specifies the component's input text color.--calcite-input-date-picker-input-placeholder-text-color
: Specifies the component's input placeholder text color.--calcite-input-date-picker-date-picker-border-color
: Specifies the component's date-picker border color.--calcite-input-date-picker-date-picker-corner-radius
: Specifies the component's date-picker border radius.--calcite-input-date-picker-date-picker-range-calendar-divider-color
: Specifies the divider color between the component's date-picker calendars whenrange="true"
.--calcite-input-date-picker-date-picker-week-header-text-color
: Specifies week header text color of the component's date-picker.--calcite-input-date-picker-date-picker-header-action-background-color
: Specifies the background color of header action's of the component's date-picker.--calcite-input-date-picker-date-picker-header-action-background-color-hover
: Specifies the background color of header action's of the component's date-picker when hovered.--calcite-input-date-picker-date-picker-header-action-background-color-press
: Specifies the background color of header action's of the component's date-picker when pressed.--calcite-input-date-picker-date-picker-header-action-text-color
: Specifies the text color of header action's of the component's date-picker.--calcite-input-date-picker-date-picker-header-action-text-color-press
: Specifies the text color of header action's of the component's date-picker when pressed.--calcite-input-date-picker-date-picker-year-text-color
: Specifies the text color of year & suffix of the component's date-picker.--calcite-input-date-picker-date-picker-month-select-font-size
: Specifies the font size of month select of the component's date-picker.--calcite-input-date-picker-date-picker-month-select-text-color
: Specifies the text color of month select of the component's date-picker.--calcite-input-date-picker-date-picker-month-select-icon-color
: Specifies the icon color of month select of the component's date-picker.--calcite-input-date-picker-date-picker-month-select-icon-color-hover
: Specifies the icon color of month select of the component's date-picker when hovered.--calcite-input-date-picker-date-picker-day-background-color
: Specifies the background color of day of the component's date-picker.--calcite-input-date-picker-date-picker-day-background-color-hover
: Specifies the background color of day of the component's date-picker when hovered.--calcite-input-date-picker-date-picker-day-text-color
: Specifies the text color of day of the component's date-picker.--calcite-input-date-picker-date-picker-day-text-color-hover
: Specifies the text color of day of the component's date-picker when hovered.--calcite-input-date-picker-date-picker-current-day-text-color
: Specifies the text color of current day of the component's date-picker.--calcite-input-date-picker-date-picker-day-background-color-selected
: Specifies the background color of selected day of the component's date-picker.--calcite-input-date-picker-date-picker-day-text-color-selected
: Specifies the text color of selected day of the component's date-picker.--calcite-input-date-picker-date-picker-day-range-text-color
: Specifies the text color of select day range of the component's date-picker.--calcite-input-date-picker-date-picker-day-range-background-color
: Specifies the background color of select day range of the component's date-picker.--calcite-input-date-picker-date-picker-day-outside-range-background-color-hover
: Specifies the background color of day's outside current range of the component's date-picker when hovered.--calcite-input-date-picker-date-picker-day-outside-range-text-color-hover
: Specifies the text color of day's outside current range of the component's date-picker when hovered.