-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Conversation
cd301a0
to
22d6d50
Compare
22d6d50
to
045afd5
Compare
const timezoneDetail = | ||
'UTC' === timezone.string | ||
? __( 'Coordinated Universal Time' ) | ||
: `(${ zoneAbbr }) ${ timezone.string.replace( '_', ' ' ) }`; | ||
: `(${ zoneAbbr }) ${ prettyTimezoneString }`; |
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.
: `(${ zoneAbbr }) ${ prettyTimezoneString }`; | |
: `(${ zoneAbbr }) ${ prettyTimezoneString }`.trim(); |
const isAbbrSameAsDetails = | ||
`${ zoneAbbr } ${ prettyTimezoneString }`.trim() === zoneAbbr; |
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.
Could we instead check for
const isAbbrSameAsDetails = | |
`${ zoneAbbr } ${ prettyTimezoneString }`.trim() === zoneAbbr; | |
const isAbbrSameAsDetails = timezoneDetail === zoneAbbr; |
?
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.
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.
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.
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.
New idea that doesn't involve making formatting changes: could we check instead that
const isAbbrSameAsDetails = | |
`${ zoneAbbr } ${ prettyTimezoneString }`.trim() === zoneAbbr; | |
const isAbbrSameAsDetails = prettyTimezoneString.trim().length === 0; |
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.
Ok. I renamed the variable and added a comment for better clarity.
Flaky tests detected in a7dd3b9. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7087119295
|
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.
🚀
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 againUTC+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