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(tooltip): deprecate the label property due to the description coming from the component's content #7247

Merged
merged 4 commits into from
Jun 30, 2023
Merged

fix(tooltip): deprecate the label property due to the description coming from the component's content #7247

merged 4 commits into from
Jun 30, 2023

Conversation

driskull
Copy link
Member

@driskull driskull commented Jun 29, 2023 •
edited
Loading

Related Issue: #6329

Summary

  • Deprecates the label property.
    • The already provided aria-describedby attribute is sufficient for accessibility.
    • Setting aria attributes on the component is not recommended.
  • Can still be set by user so no breaking changes.
  • Set to optional by default so its no longer required.
  • Update tests
  • Update Storybook
  • Update HTML

@driskull driskull marked this pull request as ready for review June 29, 2023 17:50
@driskull driskull requested a review from a team as a code owner June 29, 2023 17:50
@github-actions github-actions bot added the bug Bug reports for broken functionality. Issues should include a reproduction of the bug. label Jun 29, 2023
@driskull driskull added the pr ready for visual snapshots Adding this label will run visual snapshot testing. label Jun 29, 2023
@jcfranco
Copy link
Member

Could you update the description to include a summary of why the property is now optional & deprecated? Had to track it down in the linked issue.

jcfranco
jcfranco approved these changes Jun 29, 2023
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.

🚀

packages/calcite-components/src/components/tooltip/tooltip.tsx Outdated
/**
* Accessible name for the component.
*
* @deprecated No longer necessary.
Copy link
Member

Choose a reason for hiding this comment

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

Can we add some info on this no longer being needed because the label will come from the content/setting ARIA props on it not being recommended?

Copy link
Member

Choose a reason for hiding this comment

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

From the changelog perspective, could you also revisit the PR title to include something about this?

Copy link
Member

@jcfranco jcfranco Jun 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

WDYT about:

@deprecated The component's content provides the description, so this is no longer necessary.

?

Deferring to @geospatialem for some docspertise™.

Copy link
Member Author

Choose a reason for hiding this comment

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

How about..

@deprecated This is no longer necessary because the component's content provides the description.

I like "is" better than "should" and this ordering. Should sounds less confident :D

Copy link
Member

Choose a reason for hiding this comment

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

WDYT about some context as to what the label would provide if used? Something like:

@deprecated No longer necessary. Overrides the context of the component's description, which could confuse assistive technology users.

Copy link
Member Author

Choose a reason for hiding this comment

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

sounds good to me

@driskull driskull changed the title fix(tooltip): deprecate the label property and make it optional fix(tooltip): deprecate the label property due to the label coming from the component's content Jun 30, 2023
@driskull
Copy link
Member Author

@jcfranco @geospatialem can you review again?

@jcfranco
Copy link
Member

Just had a comment about some extra info in the @deprecated tag. Other than that, this LGTM! 🚀

@driskull driskull changed the title fix(tooltip): deprecate the label property due to the label coming from the component's content fix(tooltip): deprecate the label property due to the description coming from the component's content Jun 30, 2023
@driskull driskull merged commit 7934d75 into master Jun 30, 2023
@driskull driskull deleted the dris0000/tooltip-label branch June 30, 2023 18:27
@github-actions github-actions bot added this to the 2023 July Priorities milestone Jun 30, 2023
This was referenced Jun 30, 2023
Closed
Merged
benelan pushed a commit that referenced this pull request Aug 3, 2023
@calcite-admin @github-actions
chore: release latest (#7261)
45b5c01 
🤖 I have created a release *beep* *boop*
---


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

##
[1.5.0](https://github.com/Esri/calcite-design-system/compare/@esri/calcite-components@1.4.3...@esri/calcite-components@1.5.0)
(2023-08-03)


### Features

* **action-group:** Adds overlayPositioning property.
([#7366](#7366))
([ca9f35a](ca9f35a))
* Allow sharing focus trap stacks via configuration global
([#7334](#7334))
([934a19f](934a19f))
* Automatically import and define Calcite Components when importing
their React wrapper
([#7185](#7185))
([bf0ff67](bf0ff67))
* **block, block-section:** Add setFocus method
([#7208](#7208))
([35d4bbb](35d4bbb))
* **block:** Improve block's content layout to allow scrolling
([#7367](#7367))
([ecbf17b](ecbf17b))
* **color-picker:** Replaces thumb focus outline to rounded
([#7378](#7378))
([d803980](d803980))
* **filter:** Add filter method
([#7127](#7127))
([5a4283f](5a4283f))
* **flow:** Adds setFocus method
([#7252](#7252))
([2472c58](2472c58))
* Improve focus behavior in components
([#7277](#7277))
([ad9fbca](ad9fbca))
* **input-time-zone:** Add input-time-zone component
([#6947](#6947))
([87bd496](87bd496))
* **list:** Add slots for filter actions
([#7183](#7183))
([da07ab1](da07ab1))
* **list:** Add support for dragging items.
([#7109](#7109))
([7324f70](7324f70))
* **menu-item:** Update spacing and icon layout
([#7381](#7381))
([5659671](5659671))
* **navigation-logo:** Increase font-size of heading with no description
([#7081](#7081))
([355e101](355e101))
* **switch:** Updates focus outline to be rounded
([#7390](#7390))
([2616b82](2616b82))
* **text-area:** Provide additional context for AT users when character
limit exceeds
([#7299](#7299))
([c5678eb](c5678eb))
* **text-area:** Provide additional context for AT users when character
limit exceeds
([#7412](#7412))
([c1af3c7](c1af3c7))


### Bug Fixes

* **accordion, accordion-item:** `icon-position`, `icon-type`,
`selection-mode` and `scale` can now be set as props or attributes
([#7191](#7191))
([2b09aba](2b09aba))
* **action-bar:** No longer delegates focus when clicked on
non-focusable region
([#7310](#7310))
([1a9c15c](1a9c15c))
* **action:** Correctly focus the button after rendering updates.
([#7255](#7255))
([40fe2ce](40fe2ce))
* **block:** Loader now appears for all loading cases
([#7303](#7303))
([5af3600](5af3600))
* **block:** Removes extra loading indicator
([#7239](#7239))
([a334a75](a334a75))
* **card:** Ensure teardown logic is called when disconnected
([#7289](#7289))
([d07e322](d07e322))
* **chip:** Disconnect mutation observer when component is disconnected
from the DOM
([#7418](#7418))
([412e5fb](412e5fb))
* **color-picker:** Draw slider thumbs within bounds
([#7398](#7398))
([2f37854](2f37854))
* **color-picker:** Fix opacity slider keyboard nudging
([#7400](#7400))
([2b4f7c3](2b4f7c3))
* **color-picker:** Maintains correct numbering system when entering
invalid RGB value
([#7327](#7327))
([8d2a3a5](8d2a3a5))
* **combobox:** Add space after grouped items
([#7302](#7302))
([b1580c7](b1580c7))
* **dropdown-item:** Provides accessible label when href is not parsed
([#7316](#7316))
([966b83d](966b83d))
* **flow:** Call setFocus() on back button click
([#7285](#7285))
([9102aa4](9102aa4))
* **input-date-picker:** Provides placeholder text context for AT users
([#7320](#7320))
([31e0ba2](31e0ba2))
* **input-date-picker:** Reset active date picker date after closing
([#7219](#7219))
([91b2a1b](91b2a1b))
* **input, input-number:** No longer removes trailing decimal separator
([#7159](#7159))
([01535cf](01535cf))
* **link:** Adds outline-offset to avoid overlapping with text.
([#7342](#7342))
([c30db4e](c30db4e))
* **list:** Changing filterText property will now update filtered items
([#7133](#7133))
([a9c0bce](a9c0bce))
* **list:** Fix keyboard navigation after a listItem's disabled or
closed property changes
([#7275](#7275))
([91d28eb](91d28eb))
* **list:** Fix keyboard navigation when filterEnabled is true
([#7385](#7385))
([41a2e42](41a2e42))
* **menu-item:** Prevent duplicate border in nested vertical menu-items
([#7387](#7387))
([186a738](186a738))
* **panel:** Remove double border styling when content isn't provided
([#7368](#7368))
([91a0610](91a0610))
* Remove style modifying all host components with hidden attribute
([#7346](#7346))
([3103e2f](3103e2f))
* **scrim:** Update loader scale on resize of component.
([#7419](#7419))
([24e7f70](24e7f70))
* **slider:** Prevent excessive tick rendering
([#7421](#7421))
([c799409](c799409))
* **switch:** Fix for focus outline style in certain cases
([#7414](#7414))
([217324f](217324f))
* **tab-title:** Add full focus outline to closable tab button in high
contrast mode
([#7272](#7272))
([d812d17](d812d17))
* **tooltip:** Avoid extra before open/close event emitting
([#7422](#7422))
([dbb6818](dbb6818))
* **tooltip:** Deprecate the label property due to the description
coming from the component's content
([#7247](#7247))
([7934d75](7934d75))
* **tooltip:** Emits `close` and `beforeClose` events when container is
set to `display:none`
([#7258](#7258))
([60a4683](60a4683))
* **tooltip:** Ensure --calcite-app-z-index-tooltip is applied
([#7345](#7345))
([a9a7072](a9a7072))
</details>

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

##
[1.5.0](https://github.com/Esri/calcite-design-system/compare/@esri/calcite-components-react@1.4.3...@esri/calcite-components-react@1.5.0)
(2023-08-03)


### Features

* Automatically import and define Calcite Components when importing
their React wrapper
([#7185](#7185))
([bf0ff67](bf0ff67))


### Dependencies

* The following workspace dependencies were updated
  * dependencies
    * @esri/calcite-components bumped from ^1.5.0-next.38 to ^1.5.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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@geospatialem geospatialem geospatialem left review comments

@jcfranco jcfranco jcfranco approved these changes

Assignees

@driskull driskull

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
2023 July Priorities
Development

Successfully merging this pull request may close these issues.
3 participants
@driskull @jcfranco @geospatialem