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

fix(input-message): add missing margin to scale="s", spacing CSS variable has effect #8592

Merged
merged 5 commits into from
Jan 12, 2024
Merged

fix(input-message): add missing margin to scale="s", spacing CSS variable has effect #8592

merged 5 commits into from
Jan 12, 2024

Conversation

benelan
Copy link
Member

@benelan benelan commented Jan 12, 2024 •
edited
Loading

Related Issue: #8561 (comment)

Summary

  • Resolve CSS specificity issue that prevented --calcite-input-message-spacing-value from having any effect.
  • Add missing margin top to the small scale to align with the Figma design, where the margin is the same for all scales.

@benelan benelan requested a review from a team as a code owner January 12, 2024 04:22
@benelan benelan added the pr ready for visual snapshots Adding this label will run visual snapshot testing. label Jan 12, 2024
jcfranco
jcfranco approved these changes Jan 12, 2024
View reviewed changes
Copy link
Member

@jcfranco jcfranco left a comment

Choose a reason for hiding this comment

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

Awesome! Thanks for fixing! ✨🏆✨

packages/calcite-components/src/components/input-message/input-message.scss
}

:host([status][scale="s"]) {
:host([scale="s"]) {
Copy link
Member

Choose a reason for hiding this comment

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

Possibly out of scope, but is scale s missing an assignment to the spacing prop?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I'm not sure, maybe a question for design? What should be the spacing for the small scale @SkyeSeitz, @ashetland?

Copy link
Contributor

@ashetland ashetland Jan 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

In Figma, all three scales of Input Message have a 4px gap between the icon and text and 4px top padding.

Copy link
Contributor

Choose a reason for hiding this comment

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

@ashetland should this be a ‘—calcite-internal-‘ prop? Just want to make sure we aren’t encouraging changing this if we don’t need to.

Copy link
Member

Choose a reason for hiding this comment

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

--calcite-input-message-spacing-value is documented, so we can't change it if this was what you were suggesting.

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 deprecate it and remove in a future breaking change?

I don't understand why we'd want to be customized - a user can already add spacing to the component when used standalone.

Within a Label - they can add space to their control, and in the future when Label is baked in to form elements, all this would do is allow the space between the control and the input message to be adjusted.

Copy link
Member Author

Choose a reason for hiding this comment

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

Can we deprecate it and remove in a future breaking change?

I don't understand why we'd want to be customized - a user can already add spacing to the component when used standalone.

Within a Label - they can add space to their control, and in the future when Label is baked in to form elements, all this would do is allow the space between the control and the input message to be adjusted.

I think we should keep --calcite-input-message-spacing-value because it's the only way to modify the spacing for the validation messages I'm adding in #8574.

Although I wouldn't be opposed to deprecating it and renaming it to something like --calcite-input-message-margin-top for clarity and consistency (e.g. compared to --calcite-label-margin-bottom).

Copy link
Contributor

Choose a reason for hiding this comment

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

Modify by us or an end user? Maybe it's an -internal name if the former?

I don't think it's valid for a user to change the spacing between Input Message we render and the component, just to change the spacing. This would only make sense IMO when they add other content, which itself could handle spacing. Just seems weird to have a property to space around a component - I know we have for Label but we don't want to in the future, that should be controlled by a parent group that spaces form controls.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah that makes sense, I didn't realize we were moving away from the label spacing CSS variable. We should probably do them both at the same time in that case.

It looks like the only place --calcite-input-message-spacing-value is used internally will be in radio-button-group after #8561 is installed. So I could try to find a different way to do that, or we could change it to -internal like you mentioned.

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably fine for now, we can address when the form-components have Label functionality built in and that component is deprecated down the line.

@benelan benelan added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Jan 12, 2024
@benelan
Copy link
Member Author

benelan commented Jan 12, 2024 •
edited
Loading

@ashetland - I added the missing margin to the small scale, can you review the snapshots when you get a chance?

@benelan benelan changed the title fix(input-message): --calcite-input-message-spacing-value CSS variable takes effect fix(input-message): add missing margin to scale="s", spacing CSS variable takes effect Jan 12, 2024
@benelan benelan changed the title fix(input-message): add missing margin to scale="s", spacing CSS variable takes effect fix(input-message): add missing margin to scale="s", spacing CSS variable has effect Jan 12, 2024
@github-actions github-actions bot added the bug Bug reports for broken functionality. Issues should include a reproduction of the bug. label Jan 12, 2024
@benelan benelan added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Jan 12, 2024
@benelan benelan merged commit 49b0a20 into main Jan 12, 2024
14 checks passed
@benelan benelan deleted the benelan/input-message-spacing-value branch January 12, 2024 23:28
@calcite-admin calcite-admin mentioned this pull request Jan 12, 2024
Merged
benelan added a commit that referenced this pull request Jan 13, 2024
@benelan
Merge remote-tracking branch 'origin/main' into benelan/radio-validat…
2e8bfce 
…ion-message

* origin/main:
  fix(input-message): add missing margin to scale="s", spacing CSS variable has effect (#8592)
  feat(tile): add visual scales (#8496)
@benelan benelan mentioned this pull request Jan 13, 2024
Merged
geospatialem pushed a commit that referenced this pull request Jan 17, 2024
@calcite-admin @github-actions
chore: release main (#8455)
3709f80 
🤖 I have created a release *beep* *boop*
---


<details><summary>@esri/calcite-design-tokens: 2.1.1</summary>

##
[2.1.1](https://github.com/Esri/calcite-design-system/compare/@esri/[email protected]...@esri/[email protected])
(2024-01-17)


### Bug Fixes

* Allow users to control tabindex on interactive components
([#8166](#8166))
([b15c052](b15c052))


### Reverts

* Chore(modal): remove e2e tests that are covered by dedicated openClose
commonTests helper
([#8392](#8392))
([#8471](#8471))
([4bedf99](4bedf99))
</details>

<details><summary>@esri/calcite-components: 2.2.0</summary>

##
[2.2.0](https://github.com/Esri/calcite-design-system/compare/@esri/[email protected]...@esri/[email protected])
(2024-01-17)


### Features

* **action-menu:** Close menu on blur instead of on tab key.
([#8577](#8577))
([ccfbd0c](ccfbd0c))
* **checkbox, combobox, input-date-picker, input-time-picker,
segmented-control, select:** Add required property
([#8517](#8517))
([72a1ce4](72a1ce4))
* **handle, block, list-item:** Improve drag handle tooltip to include
item label
([#8584](#8584))
([6e643e2](6e643e2))
* **handle:** Add `blurUnselectDisabled` property to disable unselecting
handle on blur.
([#8483](#8483))
([4d665cc](4d665cc))
* **handle:** Add selected property and calciteHandleChange event.
([#8484](#8484))
([d2e9880](d2e9880))
* **list-item:** Add dragSelected property and
calciteListItemDragHandleChange event
([#8524](#8524))
([4db2eb7](4db2eb7))
* **list-item:** Add tooltip for expanding and collapsing
([#8612](#8612))
([4964491](4964491))
* **list:** Add "filter-no-results" slot to display content when no
filtered items are shown
([#8569](#8569))
([f1fc7f6](f1fc7f6))
* **list:** Introduce clearer unselected state
([#8510](#8510))
([f1e836c](f1e836c))
* **radio-button-group, segmented control:** Add validationMessage,
validationIcon, and status properties
([#8561](#8561))
([d4c5efc](d4c5efc))
* Reflect validationIcon property
([#8583](#8583))
([b3d38b3](b3d38b3))
* **table-header:** Add style when within a `selected` Table Row
([#8449](#8449))
([13cfe75](13cfe75))
* **tabs:** Emit selection-related events when selection is modified
after closing the selected tab
([#8582](#8582))
([b15c940](b15c940))
* **tile:** Add visual scales
([#8496](#8496))
([7638ec4](7638ec4))
* Use input-message to display validation messages for invalid fields
after form submission
([#8574](#8574))
([fd392fe](fd392fe))


### Bug Fixes

* **action:** Update component tokens to support transparent
([#8532](#8532))
([81cb5cc](81cb5cc))
* Allow users to control tabindex on interactive components
([#8166](#8166))
([b15c052](b15c052))
* **button:** Avoid needlessly overwriting title
([#8491](#8491))
([350a983](350a983))
* **color-picker:** Emit color change when nudging color channels by
using the shift key
([#8579](#8579))
([4250598](4250598))
* **combobox:** Only allow deleting visible chips with the keyboard
([#8603](#8603))
([2d38241](2d38241))
* **date-picker:** Prevent console error when selecting just an end date
for input date picker
([#8444](#8444))
([c0e51c3](c0e51c3))
* **filter:** Prevent console warning from displaying to end users
([#8458](#8458))
([0de7646](0de7646))
* **input-date-picker:** Ensure range icon toggles open corresponding
date-picker
([#8554](#8554))
([cfafd15](cfafd15))
* **input-date-picker:** Resolve a hard to reproduce number formatter
caching issue that occurred due to the countdown delay in queued Alerts.
([5f4fa3e](5f4fa3e))
* **input-message:** Add missing margin to scale="s", spacing CSS
variable has effect
([#8592](#8592))
([49b0a20](49b0a20))
* **input, input-number, input-text:** Restore focus on input after
browser validation error is displayed and user continues typing
([#8563](#8563))
([5897965](5897965))
* **input, input-number:** Support setting value property to Infinity
([#8547](#8547))
([f6ac698](f6ac698))
* **list-item:** Store last focused cell from focusing on elements
within a cell.
([#8494](#8494))
([28f93b4](28f93b4))
* **list, list-item, list-item-group:** Honor hidden attribute on
list-item and list-item-group
([#8541](#8541))
([3851dc6](3851dc6))
* **list:** Correct selectedItems value when list is filtered
([#8481](#8481))
([9de1922](9de1922))
* **list:** Fix event detail newIndex when down arrow pressed to sort
([#8462](#8462))
([b3d5169](b3d5169))
* **list:** Fix keyboard arrow navigation
([#8470](#8470))
([57fdaa4](57fdaa4))
* **modal:** Ensure focus trapping in dynamically created, subsequently
opened modals
([#8593](#8593))
([4ec6b94](4ec6b94))
* **table:** Fix double border on `bordered` Table Rows in
`table-footer`
([#8509](#8509))
([c16ea33](c16ea33))
* **table:** Improve Table overflow behavior
([#8424](#8424))
([79743e1](79743e1))
* **text-area:** Prevent infinite render loop when `max-length` property
is defined
([#8610](#8610))
([f30d933](f30d933))


### Reverts

* Chore(modal): remove e2e tests that are covered by dedicated openClose
commonTests helper
([#8392](#8392))
([#8471](#8471))
([4bedf99](4bedf99))


### Dependencies

* The following workspace dependencies were updated
  * devDependencies
    * @esri/calcite-design-tokens bumped from ^2.1.1-next.4 to ^2.1.1
</details>

<details><summary>@esri/calcite-components-angular: 2.2.0</summary>

##
[2.2.0](https://github.com/Esri/calcite-design-system/compare/@esri/[email protected]...@esri/[email protected])
(2024-01-17)


### Miscellaneous Chores

* **@esri/calcite-components-angular:** Synchronize undefined versions


### Dependencies

* The following workspace dependencies were updated
  * dependencies
    * @esri/calcite-components bumped from ^2.2.0-next.22 to ^2.2.0
</details>

<details><summary>@esri/calcite-components-react: 2.2.0</summary>

##
[2.2.0](https://github.com/Esri/calcite-design-system/compare/@esri/[email protected]...@esri/[email protected])
(2024-01-17)


### Miscellaneous Chores

* **@esri/calcite-components-react:** Synchronize undefined versions


### Dependencies

* The following workspace dependencies were updated
  * dependencies
    * @esri/calcite-components bumped from ^2.2.0-next.22 to ^2.2.0
</details>

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

---------

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
@calcite-admin calcite-admin mentioned this pull request May 14, 2024
Merged
@calcite-admin calcite-admin mentioned this pull request May 28, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jcfranco jcfranco jcfranco approved these changes

@macandcheese macandcheese macandcheese left review comments

@ashetland ashetland ashetland left review comments

Assignees

@benelan benelan

Labels
bug Bug reports for broken functionality. Issues should include a reproduction of the bug. pr ready for visual snapshots Adding this label will run visual snapshot testing.
Projects
None yet
Milestone
2024-01-17 - Jan Main Release
Development

Successfully merging this pull request may close these issues.
4 participants
@benelan @jcfranco @macandcheese @ashetland