Opened 2 months ago
Last modified 17 hours ago
#49295 reviewing task (blessed)
5.4 about page
Reported by: | karmatosed | Owned by: | karmatosed |
---|---|---|---|
Milestone: | 5.4 | Priority: | normal |
Severity: | normal | Version: | |
Component: | Help/About | Keywords: | needs-design has-patch dev-feedback needs-testing |
Focuses: | ui, accessibility, administration | Cc: |
Description (last modified by )
This is where discussion and work will take place during 5.4 for the about page.
Attachments (39)
Change History (125)
This ticket was mentioned in Slack in #accessibility by afercia. View the logs.
6 weeks ago
This ticket was mentioned in Slack in #accessibility by afercia. View the logs.
5 weeks ago
This ticket was mentioned in Slack in #accessibility by afercia. View the logs.
4 weeks ago
#6
@
4 weeks ago
Hi @marybaum
I noticed that you tagged with needs-patch-review
I was wondering about what version of the about.php
file you modified? I could not match it with the previous 5.3 version in trunk.
What part of the sentence should be a link?
#7
@
4 weeks ago
- Keywords needs-patch added; has-patch needs-patch-review removed
Hi @marybaum,
There's probably an issue with your patch, it only contains one string change, and also it's not generated against trunk.
#8
@
4 weeks ago
This is the draft of the copy https://docs.google.com/document/d/1huaxhgWtKSyLYKBNPtmQpPVNNvat-k9xCD60ITlloxI/edit
Awaiting final review from Josepha. @karmatosed can you remove the link in your post? Thanks!
This ticket was mentioned in Slack in #core by david.baumwald. View the logs.
4 weeks ago
@
4 weeks ago
About Page: Introduce translation strings for WP 5.4 RC 1 (and keep the existing design for the moment)
#10
@
4 weeks ago
- Keywords has-patch needs-refresh added; needs-patch removed
@melchoyce the Building more with blocks, faster and easier.
subtitle was not included in the about page draft.
Should I add it to the translation strings patch?
#12
@
4 weeks ago
Love the layout and visuals; copy is final as of review last night.
My two thoughts: would maybe like to see bigger h1 and h2 elements and possibly bigger type throughout; in my youth I used to complain bitterly about old men who wanted everything in 24px helvetica bold (course pixels were bigger then) so I’m absolutely not suggesting that—but maybe a bump on the p and then a slightly stronger hierarchy?
I also wonder if the medium blue behind the dark type should go lighter or clearer, or darker. Take that with a box of salt, though — I’m an all-brights, three-digit hex codes person, and I’m very influenced by that bias.
#13
@
4 weeks ago
Sorry, should have been more specific — what do we want to do about the release tagline that @audrasjb mentions in #comment:10?
#16
@
4 weeks ago
Hello, reviewing the content in full can I make some minor amendments? Uploaded 49295.2.diff to suggest minor changes to the privacy strings.
- Location data from the community events widget was also included with session tokens and is probably more important to the privacy focused user.
- Stating 'see where you are' is a little ambiguous as it could mean where you are on the page, in the site or otherwise. So I feel it's more informative to indicate you'll 'See progress as you process requests'
- If we're going to capitalize Privacy Tools we should capitalize both instances.
Thanks
#17
follow-up:
↓ 18
@
3 weeks ago
Hello @garrett-eclipse,
With RC1 and string freeze, we shouldn't change the current strings, as polyglots are going to start translating them in few hours… 😬
#18
in reply to:
↑ 17
@
3 weeks ago
Replying to audrasjb:
With RC1 and string freeze, we shouldn't change the current strings, as polyglots are going to start translating them in few hours… 😬
It's not uncommon to have some minor string changes after the freeze if there's a good reason for them (typo fixes, clarifications, etc.), there's an i18n-change
keyword for that.
49295.2.diff seems fine to go in after a second committer's review.
It occurred to me that 5.4 strings are not actually available for translation yet. I think @ocean90 handled this in the past. I looked around for some documentation on this, but could not find any. I'll try to do some more research tomorrow, any insights appreciated :)
#19
@
3 weeks ago
- Keywords i18n-change dev-feedback commit added
Thanks @SergeyBiryukov in that case I've added i18n-change
here. It would be nice to clarify those two privacy strings.
As you're done an initial review also marking with commit dev-feedback
to illicit the second committer's review.
#20
@
3 weeks ago
@ocean90 Thanks for making 5.4 strings available for translation!
Is this checklist still accurate? It doesn't include dotorg commits, could those be added there for completeness and future reference? Documenting all the necessary steps is one of the goals of the Release Model Working Group and would be helpful for future release leads.
For 5.5+, there are plans of clearing the milestone by Beta 1 and branching earlier, I guess that might include an earlier string freeze as well.
#21
@
3 weeks ago
I know it's string freeze. There's a minor grammar issue with a period on the inside of an HTML link
https://core.trac.wordpress.org/attachment/ticket/49295/49295.3.diff corrects it.
#22
follow-up:
↓ 23
@
3 weeks ago
I'd be okay with string changes if we can get the updated strings committed by end of this week.
I'm wondering why some strings are capitalized like "Personal Data Exports" or "Privacy Tools Tables". The latest block editor updates included many labels that where changed to sentence case (related GitHub issue).
It looks like someone really likes em dashes. Would have been better if the HTML entity —
was used to make translating strings with dashes easier. Same for apostrophes, we should have been the ’
entity. The space usage before/after the dash is a bit inconsistent.
Finally, a note about tooling. If you’re a Composer fan, version 5.4 supports this more modern tooling for PHPUnit and external libraries.
Does somebody know what change this should highlight? Based on the log there weren't any meaningful updates to the file.
#23
in reply to:
↑ 22
@
3 weeks ago
Replying to ocean90:
Finally, a note about tooling. If you’re a Composer fan, version 5.4 supports this more modern tooling for PHPUnit and external libraries.
Does somebody know what change this should highlight? Based on the log there weren't any meaningful updates to the file.
Found this in dev chat summary from January 15:
Build tools: @johnbillion is going to look at getting several of the Composer related tickets into 5.4 (like using Composer for external libraries and phpunit).
It appears to be a reference to #46815 and #48086, none of which have made it into the release, so the sentence should indeed be removed.
@
3 weeks ago
Combined patch to update privacy strings, fix period placement, update to use ’ and — (removing surrounding spacing) and drop line concerning Composer.
#24
@
3 weeks ago
- Keywords needs-refresh removed
Thanks everyone, I've combined the patches in 49295.4.diff along with a few other changes to addressed the flagged concerns.
Full list of changes;
- Updated privacy strings as mentioned in my comment:16.
- Sentenced cases the privacy strings to address @ocean90's comment:22
- Updated apostrophes to ’ entity
- Updated dashes to — entity where appropriate as well as removed spacing as they shouldn't have any surrounding space.
- Removed the string about Composer as the changes related to that didn't make this release see @SergeyBiryukov's comment:23
This should address all concerns so far mentioned in this Trac thread and will just need a final review from two committers (potentially @ocean90 and @SergeyBiryukov)
#25
@
3 weeks ago
49295.4.diff looks good to me. I'd be fine with skipping the switch to HTML entities for 5.3 and just keeping it in mind for future about pages.
#26
@
3 weeks ago
As noted on Slack by @la-geek, the new block for social icons is called "Social Icons" not "Social Links".
This ticket was mentioned in Slack in #core by ocean90. View the logs.
3 weeks ago
This ticket was mentioned in Slack in #accessibility by afercia. View the logs.
3 weeks ago
#31
@
3 weeks ago
Please check color contrast for all elements. At first glance, the one for the "tabs" seems not sufficient: it's only 3.57:1. See https://jdlsn.com/color/?type=rgb&color=255,255,255&color2=22,141,220
Checker tool courtesy of @joedolson
I have some concerns regarding the vertical text. Will propose to discuss it during next accessibility team weekly meeting.
#32
@
3 weeks ago
The look & feel is fantastic: thank you all!
I have two items that I'd like to bring up:
- This was mentioned for 5.3, but I wouldn't use the circle element around the number of the version and remove "Code is Poetry" since it's so small it's basically unreadable.
- I second @afercia concern about the vertical text. Is it necessary? I'd rather have it in a more readable, prominent way.
Thanks
#33
@
3 weeks ago
At first glance, the one for the "tabs" seems not sufficient: it's only 3.57:1.
Will fix 👍
This was mentioned for 5.3, but I wouldn't use the circle element around the number of the version and remove "Code is Poetry" since it's so small it's basically unreadable.
This is meant to be a visual identifier that ties all the release pages together; it's a reference to Blue Note albums:
- https://cdn8.openculture.com/2018/11/21232236/bluenotealbumcovers.jpg
- https://cdn.vox-cdn.com/thumbor/qRPhac-g5c63tWXb1LJFlhnYZG0=/0x0:1600x1595/1200x0/filters:focal(0x0:1600x1595):no_upscale()/cdn.vox-cdn.com/uploads/chorus_asset/file/13662666/freddie_hubbard_hub_cap_front_1600.png
- https://www.udiscovermusic.com/wp-content/uploads/2018/12/Wayne-Shorter-Speak-No-Evil-album-cover-web-optimised-740.jpg
It's not content for reading, but instead more like a sticker that denotes each page is part of a series. If people really object to it, though, we can ditch that concept moving forward.
I second @afercia concern about the vertical text. Is it necessary? I'd rather have it in a more readable, prominent way.
The text is vertical to mimic the movement of the illustration — I don't think it's nearly as effective when it's horizontal. You can see some alternate options in Figma: https://www.figma.com/file/Haqdaz1jlMhSRdhhSDxbIE/About-Page-5.4?node-id=0%3A1, but, they feel like much weaker compositions to me.
#34
@
3 weeks ago
Changes based on feedback:
As Mel pointed out, the vertical label WordPress 5.4 and the Blue Note Album notation are both more visual than lexical elements -- and you've got to know how much that word lexical is killing me, the copy hack and wannabe standup comedian, to write. But I'll get over it.
I have always understood this top illustration as evoking piano keys, and in this version those keys play the music over all else.
As a [design] squad, we're willing to let go of the album notation entirely if there's still heavy opposition; we do still feel pretty strongly about the vertical WordPress 5.4.
A further note: the oval is part of the standard format of the album notation -- I point it out only because it's easy to miss in the examples Mel cites. I missed it until she pointed them out explicitly! Course, I have never claimed to be a musical sophisticate.
In a nod toward readability, you'll notice the tag line, "Building more with blocks, faster and easier." is bigger. It starts directly in line with the Privacy nav link and extends to the natural right margin of the rest of the type.
#35
follow-up:
↓ 36
@
2 weeks ago
https://build.trac.wordpress.org/browser/trunk/wp-admin/about.php?marks=172#L172
<li><?php _e( 'In embeds, now the block editor supports TikTok—and CollegeHumor is gone.' ); ?></li>
CollegeHumor is gone.
CollegeHumor has gone in WP 5.3.1 not within 5.4.
#36
in reply to:
↑ 35
;
follow-up:
↓ 37
@
2 weeks ago
- Keywords commit removed
Replying to La Geek:
CollegeHumor has gone in WP 5.3.1 not within 5.4.
It was removed from the Block Editor within WP 5.4.
The Pull Request on GitHub was slated to Gutenberg 7.0: https://github.com/WordPress/gutenberg/pull/18591
#37
in reply to:
↑ 36
;
follow-up:
↓ 38
@
2 weeks ago
Replying to audrasjb:
Replying to La Geek:
CollegeHumor has gone in WP 5.3.1 not within 5.4.
It was removed from the Block Editor within WP 5.4.
The Pull Request on GitHub was slated to Gutenberg 7.0: https://github.com/WordPress/gutenberg/pull/18591
@audrasjb
Please open 5.3.2 without Plugin Gutenberg. It is not embedded.
See also: https://core.trac.wordpress.org/search?q=college
#38
in reply to:
↑ 37
@
2 weeks ago
Replying to La Geek:
Please open 5.3.2 without Plugin Gutenberg. It is not embedded.
See also: https://core.trac.wordpress.org/search?q=college
Yes, the embed provider was removed in #48696 during a point release, but the embed Block was slated removed on Gutenberg 7.0, which was fully merged in core with WordPress 5.4, that's why the related dev note was published on Make/Core for 5.4. I'm not opposed to remove this item from the about page, but I'd say it's better to stay consistent with the Gutenberg releases and the related dev notes :)
#39
@
2 weeks ago
In the second design concept, "Code Is Poetry" is larger, but the typo remains (poery).
#40
follow-up:
↓ 49
@
10 days ago
49295.5.diff implements the latest mockup styles, switches up the embedded svgs, and reorganizes some HTML (but no string changes).
In this patch, the header image is a background image pulled from the local uploads, so it won't work until that lives somewhere real. I think the header image SVG should be uploaded to the CDN, since it's a bit large for embedding into the CSS, but could go either way.
I also added the badge (diagonal "code is poetry") into the header background image, since it's meant as a decorative element.
#41
@
10 days ago
Thanks @ryelle !
I noticed small bugs in some specific contexts
49295.6.diff
addresses a vertical overflow issue on screens with small height (like when you use the browser inspector).
This ticket was mentioned in Slack in #core by karmatosed. View the logs.
10 days ago
#45
@
10 days ago
@audrasjb I can't replicate that (also on Mac, tried FF, Chrome & Safari)— could you have the CSS cached, maybe?
This ticket was mentioned in Slack in #accessibility by sabernhardt. View the logs.
8 days ago
#49
in reply to:
↑ 40
@
8 days ago
Replying to ryelle:
In this patch, the header image is a background image pulled from the local uploads, so it won't work until that lives somewhere real. I think the header image SVG should be uploaded to the CDN, since it's a bit large for embedding into the CSS, but could go either way.
The image has been uploaded to WordPress.org, but [47475] didn't use a CDN URL just in case it needs any updates before the final release. The URL should be replaced with a CDN domain (s.w.org
) before the final release. This is now added to the Pre Final Release checklist.
#52
@
8 days ago
Testing the committed changes I found the Field Guide URL was using the p=### format and not it's permalink. In 49295.7.diff I've updated to use the permalink;
https://make.wordpress.org/core/2020/03/03/wordpress-5-4-field-guide/
Also concerning use of the s.w.org CDN can we not just switch them to encoded data-uri's to avoid the external call?
#53
follow-ups:
↓ 57
↓ 58
@
8 days ago
One other question while I tested on mobile, Should the footer contents vanish? Speaking of the 'Thank you for creating with WordPress.' message and version information found at the base of the page.
#54
@
6 days ago
49295.8.diff fixes a typo in the about page.
#55
@
4 days ago
Hi @garrett-eclipse, @ixkaito thank you for patches https://core.trac.wordpress.org/attachment/ticket/49295/49295.7.diff and https://core.trac.wordpress.org/attachment/ticket/49295/49295.8.diff both of them were committed.
#57
in reply to:
↑ 53
@
4 days ago
Replying to garrett-eclipse:
One other question while I tested on mobile, Should the footer contents vanish? Speaking of the 'Thank you for creating with WordPress.' message and version information found at the base of the page.
Do they on other pages on mobile? We should copy whatever the rest of the admin does.
#58
in reply to:
↑ 53
@
4 days ago
Replying to garrett-eclipse:
One other question while I tested on mobile, Should the footer contents vanish? Speaking of the 'Thank you for creating with WordPress.' message and version information found at the base of the page.
Thanks for bringing this up! Per these lines in wp-admin/css/common.css, hiding the footer on mobile on all admin pages does seem to be intentional, so it's not a regression. Introduced in [26134].
#59
@
4 days ago
Thanks for the commit @jorgefilipecosta.
And I appreciate the response @melchoyce & @SergeyBiryukov it appears it was intentional, although scanning the ticket I see no mention as to why. Seems an odd choice so will do some more digging and maybe look to re-introduce that helpful information to mobile users in the future.
@
3 days ago
header: moving h1 tag to other text while maintaining previous styles, editing breakpoint for mobile layout, and adding fixes for Internet Explorer and/or RTL languages
#60
@
3 days ago
49295.9.diff has some changes for the header section:
- The vertical text was intended as more visual than informational, but then it was placed inside a heading tag. The H1 would make it the most important text on the page.
However, it could be better to move the H1 tag anyway, so that it would be unique to each page. These choices seemed most logical to me:
About (What's New): "Say hello to more and better." (add Maintenance Releases section below this intro section later)
Credits: "WordPress is created by a worldwide team of passionate individuals."
Freedoms: "Freedoms"
Privacy: "Privacy"
The stylesheet is revised to match the current styling for "WordPress 5.4" as well as the first heading within the content, but with the new tags.
- With the blue rectangles so close to the edge of the 600×600 background image, text can overlap the rectangles. The patch removes the 600px breaking point for header elements, so that layout starts at 782px instead. (This makes the header section taller at those widths, but it's a simpler option than editing either the SVG or the
background-size
property.)
- The
max-height: none
replacesunset
for Internet Explorer at a high zoom level and/or narrow browser width. The vertical text was switching direction from up to down instead of switching to horizontal, which pushed the other header content on top of later content. I also addedclear: both
to the section containers to prevent overlapping.
- Again for IE11,
-ms-writing-mode
neededlr-tb
(andrl-tb
) instead ofinitial
. It also includes thedirection
property.
- The RTL-specific styles remove the 180-degree rotation to align the text to the outer edge.
#61
@
28 hours ago
- Keywords needs-testing added
The last patch could use some further testing yet :)
If you're curious about the Arabic "Building more..." text, I needed to add that manually. (The translation was approved, but apparently after the most recent package was made available through the Updates page.)
This ticket was mentioned in Slack in #core-css by sabernhardt. View the logs.
26 hours ago
This ticket was mentioned in Slack in #accessibility by afercia. View the logs.
25 hours ago
#64
@
24 hours ago
In today's accessibility chat it was raised that the current patch to change away from a heading is still something they would advise isn't far enough. @melchoyce stepped up and created a rapid iteration as shown here:
Whilst, we are in release candidate, as this doesn't change strings it can be something to bring in. @audrasjb is being gracious enough to work on a patch.
The tagline is to be seen as a visual element in this case, which is a middle ground that keeps the art direction but iterates based on feedback the release title.
#65
@
24 hours ago
I was just testing & updating 49295.9.diff, and was about to post an updated patch— will the headings still need to be changed, or will this be an entirely new patch? There are other fixes in 49295.9.diff that should still come across.
#66
@
23 hours ago
Well, in any case… I've uploaded two patches, 49295-heading-changes.diff iterates on 49295.9.diff to make the heading level changes consistent across all screens.
And if we don't need the heading change anymore, 49295-no-heading-changes.diff pulls out the non-heading related changes from 49295.9.diff so they're easier to merge into the patch @audrasjb is working on.
@
23 hours ago
About page: remove text rotation on WordPress version and useless <em> tag as per accessibility team meeting statements
#67
@
23 hours ago
- Keywords i18n-change removed
Hi,
49295.10.diff
implements 49295.9.diff
and:
- removes the 180deg rotation on WordPress version text
- removes an useless
<em>
tag as per accessibility team statement
No string change, please help testing this patch!
#68
@
22 hours ago
I am seeing the "WordPress 5.4" much smaller now. I think 49295.10.diff is missing the CSS changes that reference the old H1 in favor of the new P heading. Confirmed by @karmatosed in testing as well. Screenshot incoming.
#69
@
21 hours ago
49295.11.diff integrates the changes from 49295-heading-changes.diff, resets the font size on 5.4
, and removes the un-rotating CSS from the media queries (since we're not rotating the text in the first place).
@davidbaumwald Can you try a hard refresh? sometimes the CSS caching is a bit aggressive.
#70
@
21 hours ago
@ryelle Yes, I did a hard refresh. I think is was just an issue of some missing CSS in the 49295.10.diff patch, and it's being reworked now. ;) Thanks!
#71
@
21 hours ago
@davidbaumwald You could try with 49295.11.diff? that should have all the appropriate CSS.
#72
@
20 hours ago
49295.12.diff builds on 49295.11.diff with one change:
- Readjust the logic in
credits.php
to avoid displaying two similar "WordPress is created by..." strings next to each other if the Credits API response is unavailable.
Still seeing one minor issue with it: "WordPress 5.4" overlaps with the image on smaller screen resolutions, see 49295.12.overlapping.png. The initial mockup in comment:64 didn't have this issue as there was some vertical space between the image and the "WordPress 5.4" text. Currently trying to address that, any help appreciated.
#73
@
20 hours ago
There is still a minor issue, but this is not a blocker in my opinion.
For what it's worth, I'd sign-off for shipping 49295.12
as it is @SergeyBiryukov 🙂
#74
@
19 hours ago
Concerning the minor issue, I think one plan would be to handle positioning breakpoint by breakpoint. We could also rethink the CSS of the block to place the text after/at the bottom of the SVG image. But this would need a full refresh of the CSS of this block. Not sure it's doable in our timeframe.
#75
@
19 hours ago
I didn't test it for now, but the current small screen media query uses a padding-top: 80%
. Maybe we could use something like that to ensure the text is at the bottom of the svg image?
#76
@
19 hours ago
.about__header {padding-top: 10%;}
seems to work well enough for me to try in a patch :)
...which then would need to be removed so it doesn't add to the current 80% on the title div
#77
@
19 hours ago
I didn't test it for now, but the current small screen media query uses a padding-top: 80%. Maybe we could use something like that to ensure the text is at the bottom of the svg image?
That only works because it's not flex
ed anymore— we can update the SVG to be slightly taller, which works to give the necessary space.
#78
@
19 hours ago
.about__header { padding-top: 10%; }
didn't seem to work in my testing.
Took another approach in 49295.14.diff with a few breakpoints for the font size:
@media screen and (max-width: 1280px) { .about__header-title p { font-size: 3.4em; } } @media screen and (max-width: 1024px) { .about__header-title p { font-size: 3em; } } @media screen and (max-width: 960px) { .about__header-title p { font-size: 2.7em; } }
That seems to work and resolves the overlapping.
#81
follow-up:
↓ 84
@
17 hours ago
49295.16.diff adds a new header image, along with an RTL version, to prevent the overlap. Now that we don't need to line up text, this also removes the unnecessary grid-styling. It should look like this: LTR page, RTL page.
The images will need to be uploaded & renamed in the patch.
#82
@
17 hours ago
- Owner set to audrasjb
- Status changed from new to reviewing
I tested 49295.16.diff
and it looks great to me, thanks @ryelle !
Strings patch for aout.php