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.

Sign up for GitHub

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

Jump to bottom

feat(action): add component tokens #8836

Merged
merged 18 commits into from
Mar 1, 2024
Merged

feat(action): add component tokens #8836

merged 18 commits into from
Mar 1, 2024

Conversation

alisonailea
Copy link
Contributor

@alisonailea alisonailea commented Feb 27, 2024 •
edited
Loading

Related Issue: #7180

Summary

Action

--calcite-action-indicator-color: Specifies the color of the component's indicator.
--calcite-action-background-color: Specifies the background color of the component.
--calcite-action-text-color: Specifies the text color of the component.

Action Bar

--calcite-action-bar-action-indicator-color: defines the indicator color of an action sub-component inside the component.
--calcite-action-bar-action-background-color: defines the background color of an action sub-component inside the component.
--calcite-action-bar-action-text-color: defines the text color of an action sub-component inside the component.

Action Group

--calcite-action-group-action-indicator-color: defines the indicator color of an action sub-component inside the component.
--calcite-action-group-action-background-color: defines the background color of an action sub-component inside the component.
--calcite-action-group-action-text-color: defines the text color of an action sub-component inside the component.

Action Menu

--calcite-action-menu-action-indicator-color: defines the indicator color of an action sub-component inside the component.
--calcite-action-menu-action-background-color: defines the background color of an action sub-component inside the component.
--calcite-action-menu-action-text-color: defines the text color of an action sub-component inside the component.

Flow Item

--calcite-flow-item-action-indicator-color: defines the indicator color of an action sub-component inside the component.
--calcite-flow-item-action-background-color: defines the background color of an action sub-component inside the component.
--calcite-flow-item-action-text-color: defines the text color of an action sub-component inside the component.

Menu Item

--calcite-menu-item-action-indicator-color: defines the indicator color of an action sub-component inside a menu-item.
--calcite-menu-item-action-background-color: defines the background color of an action sub-component inside a menu-item.
--calcite-menu-item-action-text-color: defines the text color of an action sub-component inside a menu-item.

Navigation

--calcite-navigation-action-indicator-color: defines the indicator color of an action sub-component inside the component.
--calcite-navigation-action-background-color: defines the background color of an action sub-component inside the component.
--calcite-navigation-action-text-color: defines the text color of an action sub-component inside the component.

Panel

--calcite-panel-action-indicator-color: defines the indicator color of an action sub-component inside the component.
--calcite-panel-action-background-color: defines the background color of an action sub-component inside the component.
--calcite-panel-action-text-color: defines the text color of an action sub-component inside the component.

Popover

--calcite-popover-action-indicator-color: defines the indicator color of an action sub-component inside the component.
--calcite-popover-action-background-color: defines the background color of an action sub-component inside the component.
--calcite-popover-action-text-color: defines the text color of an action sub-component inside the component.

Stepper

--calcite-stepper-action-indicator-color: defines the indicator color of an action sub-component inside the component.
--calcite-stepper-action-background-color: defines the background color of an action sub-component inside the component.
--calcite-stepper-action-text-color: defines the text color of an action sub-component inside the component.

Tip Manager

--calcite-tip-manager-action-indicator-color: defines the indicator color of an action sub-component inside the component.
--calcite-tip-manager-action-background-color: defines the background color of an action sub-component inside the component.
--calcite-tip-manager-action-text-color: defines the text color of an action sub-component inside the component.

Tip

--calcite-tip-action-indicator-color: defines the indicator color of an action sub-component inside the component.
--calcite-tip-action-background-color: defines the background color of an action sub-component inside the component.
--calcite-tip-action-text-color: defines the text color of an action sub-component inside the component.

@alisonailea alisonailea added the design-tokens Issues requiring design tokens. label Feb 27, 2024
@alisonailea alisonailea changed the base branch from main to epic/7180-component-tokens February 27, 2024 16:53
@github-actions github-actions bot added the enhancement Issues tied to a new feature or request. label Feb 27, 2024
@alisonailea alisonailea requested a review from driskull February 27, 2024 18:20
@alisonailea
docs(action-bar, action-group, action, flow-item, input, menu-item, n…
55ecba7 
…avigation, panel, popover, stepper, tip-manager, tip): sort docs alphabetically
@alisonailea
test(action): fix selectors
b90edd8
@alisonailea
fix(action): add back mixin
f76e648
@alisonailea
Merge branch 'epic/7180-component-tokens' of github.com:Esri/calcite-…
ee2c1f5 
…components into astump/7180-action

# Conflicts:
#	packages/calcite-components/src/components/action-group/action-group.scss
#	packages/calcite-components/src/components/action-menu/action-menu.scss
#	packages/calcite-components/src/components/menu-item/menu-item.scss
#	packages/calcite-components/src/components/panel/panel.scss
@alisonailea alisonailea requested a review from driskull February 29, 2024 01:26
driskull
driskull requested changes Feb 29, 2024
View reviewed changes
Copy link
Member

@driskull driskull left a comment

Choose a reason for hiding this comment

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

Just some more cleanup then it should be good to go.

A class is preferred instead of an element selector incase another button element is added inside the component for some state. Its better to be specific.

packages/calcite-components/src/components/action/action.scss Outdated Show resolved Hide resolved
packages/calcite-components/src/components/action/action.scss Outdated Show resolved Hide resolved
packages/calcite-components/src/components/action/action.scss Outdated Show resolved Hide resolved
packages/calcite-components/src/components/action/action.scss Outdated Show resolved Hide resolved
packages/calcite-components/src/components/action/action.scss Outdated Show resolved Hide resolved
packages/calcite-components/src/components/action/action.scss Show resolved Hide resolved
packages/calcite-components/src/components/action/action.scss Show resolved Hide resolved
packages/calcite-components/src/components/action/action.scss Show resolved Hide resolved
packages/calcite-components/src/components/action/action.scss Show resolved Hide resolved
packages/calcite-components/src/components/action/action.scss Show resolved Hide resolved
@alisonailea
Copy link
Contributor Author

Just some more cleanup then it should be good to go.

A class is preferred instead of an element selector incase another button element is added inside the component for some state. Its better to be specific.

No it's not. This is a coding approach recommended for large React applications and LightDom elements, not web components. There is no use case in which a simple action element should have more than one button.

@driskull
Copy link
Member

No it's not. This is a coding approach recommended for large React applications and LightDom elements, not web components. There is no use case in which a simple action element should have more than one button.

It seems like we just need to clarify in our coding guidelines for when its appropriate to use an element as a selector vs a class. In action, it may be perfectly fine since there will likely only be one button element ever inside. However, for a more complex component that would not be the same. I think we need to document when to do one vs the other.

See:
https://github.com/Esri/calcite-design-system/blob/main/packages/calcite-components/conventions/README.md#css-class-names

We also have this guideline which we've been not really doing:
https://github.com/Esri/calcite-design-system/blob/main/packages/calcite-components/conventions/Styling.md#styling

cc @jcfranco

@alisonailea
revert(action): remove CSS selectors
b20ac85
@alisonailea
fix(tip, tip-manager, stepper, popover, panel, navigation, menu-item,…
202af98 
… flow-item, action-menu, action-group, action-bar): add mapped state tokens for action when a dependency
driskull
driskull approved these changes Feb 29, 2024
View reviewed changes
Copy link
Member

@driskull driskull left a comment

Choose a reason for hiding this comment

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

Changes look good to me once other feedback is addressed 🎉 🌮

@alisonailea
Merge branch 'epic/7180-component-tokens' of github.com:Esri/calcite-…
98f1161 
…components into astump/7180-action

# Conflicts:
#	packages/calcite-components/src/components/input/input.scss
@jcfranco
Copy link
Member

jcfranco commented Mar 1, 2024 •
edited
Loading

No it's not. This is a coding approach recommended for large React applications and LightDom elements, not web components. There is no use case in which a simple action element should have more than one button.

It seems like we just need to clarify in our coding guidelines for when its appropriate to use an element as a selector vs a class. In action, it may be perfectly fine since there will likely only be one button element ever inside. However, for a more complex component that would not be the same. I think we need to document when to do one vs the other.

@alisonailea @driskull Let’s discuss this after we wrap up #7180. Minor cleanup and collateral refactoring are expected, but the changing of classes for element selectors are slightly beyond what’s required for the issue.

I’m open to adjusting our approach. Here’s a quick overview on our current pattern:

• it provides an abstraction layer that helps refactor the component internals without affecting styles and testing
• we don’t have to worry about the complexity of a component; we treat them all the same, which keeps things straightforward
• it is semantic
• it is a common web component pattern (either class or ID):

@alisonailea alisonailea merged commit cb0aabb into epic/7180-component-tokens Mar 1, 2024
4 checks passed
@alisonailea alisonailea deleted the astump/7180-action branch March 1, 2024 17:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@driskull driskull driskull approved these changes

@benelan benelan Awaiting requested review from benelan

@jcfranco jcfranco Awaiting requested review from jcfranco

@geospatialem geospatialem Awaiting requested review from geospatialem

@macandcheese macandcheese Awaiting requested review from macandcheese

Assignees

@alisonailea alisonailea

Labels
design-tokens Issues requiring design tokens. enhancement Issues tied to a new feature or request.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@alisonailea @driskull @jcfranco @macandcheese