-
Notifications
You must be signed in to change notification settings - Fork 89
feat: add internal label across form components #12499
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
Changes from all commits
1c0d644
dc6b904
5ed7e17
0d74296
a513f7a
cbd0167
6502a47
01fe6e3
507e3cd
9479fb0
f619f62
15d5283
c6c4457
abde880
ed99785
0295f03
4d2d7b5
df1a156
8bf0381
7422503
a77ea50
be2bb3d
bf21d7d
43a8d40
1e8d3e8
d776f6c
5e5f0ec
9f03b2d
31ee3fd
9adc38e
a8997f5
61ceaf9
54636b2
9843bf4
27e99df
dff28c8
c0b3073
474db76
71f7270
a7449c6
8fd0ff0
41d0c8d
43ddcb7
8edd23a
3b2a6b0
ef28bd6
d1e623e
7b8ae84
48b4473
3503459
2695bcb
74c87d5
66ebc5c
062068f
267a98e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -36,6 +36,81 @@ | |
| } | ||
| } | ||
|
|
||
| // mixin for the container of label displayed with form-associated components | ||
| @mixin form-internal-label() { | ||
| .internal-label-alignment--center { | ||
| align-items: center; | ||
| } | ||
|
|
||
| .internal-label-alignment--end { | ||
| align-items: end; | ||
| } | ||
|
|
||
| .internal-label--container { | ||
| display: flex; | ||
| justify-content: space-between; | ||
| color: var(--calcite-color-text-1); | ||
| } | ||
|
|
||
| .internal-label-required--indicator { | ||
| font-weight: var(--calcite-font-weight-medium); | ||
| color: var(--calcite-color-status-danger); | ||
| padding-inline: var(--calcite-spacing-base); | ||
| &:hover { | ||
| cursor: help; | ||
| } | ||
| } | ||
|
|
||
| .internal-label--text { | ||
| line-height: 1; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this use a token?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I found these tokens with value of var(--calcite-opacity-100)
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @aPreciado88 it should be a var for line height. We can't just use any @ashetland can you chime in? These are the ones I see: https://developers.arcgis.com/calcite-design-system/foundations/tokens/reference/#font
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As I recall, we were unable to get the correct alignment using our tokens. @aPreciado88 did we try
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ashetland I tried
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. #12783 created. To confirm, the relative line-height additions listed in this issue are all we need here?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looking at the compiled css for Calcite, it does seem like this is a bug:
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @driskull my issue proposes changing
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes that seems correct 👍 |
||
| } | ||
|
|
||
| :host([scale="s"]) { | ||
| .internal-label-spacing--bottom { | ||
| margin-block-end: var(--calcite-spacing-xxs); | ||
| } | ||
| .internal-label-spacing-inline--end { | ||
| margin-inline-end: var(--calcite-spacing-sm); | ||
| } | ||
| .internal-label-spacing-inline--start { | ||
| margin-inline-start: var(--calcite-spacing-sm); | ||
| } | ||
| .internal-label--text { | ||
| font-size: var(--calcite-font-size--2); | ||
| } | ||
| } | ||
|
|
||
| :host([scale="m"]) { | ||
| .internal-label-spacing--bottom { | ||
| margin-block-end: var(--calcite-spacing-sm); | ||
| } | ||
| .internal-label-spacing-inline--end { | ||
| margin-inline-end: var(--calcite-spacing-sm); | ||
| } | ||
| .internal-label-spacing-inline--start { | ||
| margin-inline-start: var(--calcite-spacing-sm); | ||
| } | ||
| .internal-label--text { | ||
| font-size: var(--calcite-font-size--1); | ||
| } | ||
| } | ||
|
|
||
| :host([scale="l"]) { | ||
| .internal-label-spacing--bottom { | ||
| margin-block-end: var(--calcite-spacing-sm); | ||
| } | ||
| .internal-label-spacing-inline--end { | ||
| margin-inline-end: var(--calcite-spacing-md); | ||
| } | ||
| .internal-label-spacing-inline--start { | ||
| margin-inline-start: var(--calcite-spacing-md); | ||
| } | ||
| .internal-label--text { | ||
| font-size: var(--calcite-font-size-0); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // mixin for the container of validation messages displayed below form-associated components | ||
| @mixin form-validation-message() { | ||
| .validation-container { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -415,4 +415,3 @@ export const emptyHeader = (): string => html` | |
| </calcite-label> | ||
| </calcite-block> | ||
| `; | ||
|
|
||

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.
Just confirming this cursor usage cc @SkyeSeitz @ashetland
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.
We had mentioned it in the design file, but don't have to move forward with it if we don't want to.