-
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
Block Directory: Change 'update' icon to text to be more communicative. #19451
Block Directory: Change 'update' icon to text to be more communicative. #19451
Conversation
@@ -16,7 +16,9 @@ function DownloadableBlockInfo( { description, activeInstalls, humanizedUpdated | |||
<Icon icon="chart-line"></Icon>{ sprintf( _n( '%d active installation', '%d active installations', activeInstalls ), activeInstalls ) } | |||
</div> | |||
<div className="block-directory-downloadable-block-info__column"> | |||
<Icon icon="update"></Icon><span aria-label={ sprintf( __( 'Updated %s' ), humanizedUpdated ) }>{ humanizedUpdated }</span> | |||
<span aria-label={ sprintf( __( 'Updated %s' ), humanizedUpdated ) }> | |||
<strong>{ __( 'Updated: ', 'gutenberg' ) }</strong> { humanizedUpdated } |
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.
<strong>{ __( 'Updated: ', 'gutenberg' ) }</strong> { humanizedUpdated } | |
<strong>{ __( 'Updated: ' ) }</strong> { humanizedUpdated } |
We don't use the gutenberg
textdomain here, because it will be merged into core.
@@ -16,7 +16,9 @@ function DownloadableBlockInfo( { description, activeInstalls, humanizedUpdated | |||
<Icon icon="chart-line"></Icon>{ sprintf( _n( '%d active installation', '%d active installations', activeInstalls ), activeInstalls ) } | |||
</div> | |||
<div className="block-directory-downloadable-block-info__column"> | |||
<Icon icon="update"></Icon><span aria-label={ sprintf( __( 'Updated %s' ), humanizedUpdated ) }>{ humanizedUpdated }</span> | |||
<span aria-label={ sprintf( __( 'Updated %s' ), humanizedUpdated ) }> |
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.
Without an icon I don't think this span
with aria-label
makes too much sense anymore. A11y feedback would be good here.
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.
Definitely agreed, otherwise the aria-label will override the actual text. And since the actual text is even more descriptive now, that would mean a loss of information. So yes, the span should go away including the aria-label. The strong with info text is totally sufficient here.
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.
Yeah totally. Thanks for the heads up!
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.
In a parallel conversation, I was made aware of these prototypes:
https://www.figma.com/file/QKhoOKXkBN2mHacqvrtdNuI6/Gutenberg-Block-Library-Installation?node-id=370%3A0
It appears like the current implementation was missing the "Updated" string but maintained the copy. The design could accommodate it by having a smaller font size. With that in mind. I have updated this PR to be:
Should this be tagged with "Needs Design Feedback" in case there have been other conversations had here?
…extra elements, make the font size smaller.
LGTM, but due to the visuals, don't feel I have the authority to approve this. |
Yep, totally with you on this. Let me try a bit of tag cloud to pull in some more opinions :) |
Does the label wrap to a new line in longer translations? Want to make sure it doesn't cut off. |
It will not be cut off. However since we use |
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.
👍 Good from a design perspective.
Description
The 'update' dashicon does not do a very good job communicating the "Last Update" date. This PR changes that icon to a copy. This brings in more alignment with the Plugin Directory and its UI as well.
How has this been tested?
Tested manually.
Screenshots
Before:
After:
Types of changes
Copy Update.
Checklist: