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.

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

Use tooltip for the Timezone only when necessary. #56214

Merged
merged 3 commits into from
Dec 5, 2023

Conversation

afercia
Copy link
Contributor

@afercia afercia commented Nov 16, 2023

Fixes #56211

What?

The Timezone indication may render an unnecessary tooltip when the visible text and the tooltip text are the same.

Why?

There is no need to repeat in the tooltip the same information that is already in the visible text e.g. UTC+3 and again UTC+3.

How?

Checks whether the abbreviation and the detailed values contain the same information and add the tooltip conditionally.

Testing Instructions

Testing Instructions for Keyboard

Screenshots or screencast

@afercia afercia added [Type] Bug An existing feature does not function as intended [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Package] Components /packages/components labels Nov 16, 2023
@afercia afercia force-pushed the fix/timezone-redundant-tooltip branch from cd301a0 to 22d6d50 Compare November 21, 2023 14:20
@afercia afercia marked this pull request as ready for review November 21, 2023 14:21
const timezoneDetail =
'UTC' === timezone.string
? __( 'Coordinated Universal Time' )
: `(${ zoneAbbr }) ${ timezone.string.replace( '_', ' ' ) }`;
: `(${ zoneAbbr }) ${ prettyTimezoneString }`;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
: `(${ zoneAbbr }) ${ prettyTimezoneString }`;
: `(${ zoneAbbr }) ${ prettyTimezoneString }`.trim();

Comment on lines 43 to 44
const isAbbrSameAsDetails =
`${ zoneAbbr } ${ prettyTimezoneString }`.trim() === zoneAbbr;
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we instead check for

Suggested change
const isAbbrSameAsDetails =
`${ zoneAbbr } ${ prettyTimezoneString }`.trim() === zoneAbbr;
const isAbbrSameAsDetails = timezoneDetail === zoneAbbr;

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would not work because timezoneDetail may contain the parenthesis around zoneAbbr in fact the string in the ternary above contains parenthesis ( ... ) so that the comparison would happen between, for example:
(UTC+5.30) and UTC+5.30 and thus be falsy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could entirely remove the parenthesis maybe? They are only used in the Tooltip:

Screenshot 2023-11-30 at 12 59 46

This is how it would look without parenthesis:

Screenshot 2023-11-30 at 13 02 53

Copy link
Contributor

Choose a reason for hiding this comment

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

New idea that doesn't involve making formatting changes: could we check instead that

Suggested change
const isAbbrSameAsDetails =
`${ zoneAbbr } ${ prettyTimezoneString }`.trim() === zoneAbbr;
const isAbbrSameAsDetails = prettyTimezoneString.trim().length === 0;

Copy link
Contributor Author

@afercia afercia Dec 4, 2023

Choose a reason for hiding this comment

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

Ok. I renamed the variable and added a comment for better clarity.

Copy link

github-actions bot commented Dec 4, 2023

Flaky tests detected in a7dd3b9.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7087119295
📝 Reported issues:

Copy link
Contributor

@ciampo ciampo left a comment

Choose a reason for hiding this comment

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

🚀

@afercia afercia merged commit 743e0b6 into trunk Dec 5, 2023
52 checks passed
@afercia afercia deleted the fix/timezone-redundant-tooltip branch December 5, 2023 07:43
@github-actions github-actions bot added this to the Gutenberg 17.3 milestone Dec 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Package] Components /packages/components [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TimeZone: Do not show a tooltip that just repeats the visible text
2 participants