Podcast Player: Clean-up styles & fix few remaining issues by WunderBart · Pull Request #15512 · Automattic/jetpack

WunderBart · 2020-04-21T17:47:09Z

Built on top of #15504 as a part of completing the remaining Podcast Player improvements. My main goal was to go through styles.scss and clean it up a bit to minimize technical debt. On the way, I found a few small bugs and decided not to create separate issues for them as they kind of fixed themselves while cleaning up the code :)

Changes proposed in this Pull Request:

Maintenance:

Removed duplicated SCSS variables,
Renamed some SCSS variables for clarity,
Used available SCSS variables for hardcoded values where possible,
Unified gutter and some paddings into simple $gutter-s/m/l vars,
Cleaned up and made colors styles more consistent and easy to follow,
Cleaned up a few misplaced comments.

Bugfixes:

Made links styling consistent (Podcast title, Track title, Error message)
Links have the Secondary color initially and Primary when hovered (except for an active Track, which has Primary color at all times). Previously that was true only when default colors were applied. Now it's true also for any custom colors thanks to progressive enhancement with the CSS vars. Note that therefore hover colors are not supported in, i.e. IE11.
Fixed error message link ("Open in a new tab") to which custom colors weren't applied in browsers that don't support CSS vars (IE11).
Used correct default, $dark-gray-300 (Secondary) color for episode description text instead of $dark-gray-500.
Made the ME.js progress bar tooltip's text color the same as the background for proper contrast.

One enhancement:

Removed block border (p1587552447434800-slack-ajax)

Testing instructions:

Add the Podcast Player block (use e.g. this URL https://anchor.fm/s/9400d7c/podcast/rss),
Make sure that podcast title, inactive track and the "Open in a new tab" track error links have all correct colors - Secondary initially and Primary on hover (hover colors applied only for browsers that support CSS variables),
Make sure that episode description's default color is correct (#6c7781),
Make sure the block has no border,
Make sure the ME.js progress bar tooltip's text color is the same as the background,
All of the above should be true when switching themes and global styles.

Demo GIF:

Colors: custom
Theme: Varia
Global styles: "System Font & Libre Baskerville"

IE11 screenshots (same for editor and front-end):

playback	track error

No-JS screenshot (front-end):

Colors: custom
Theme: Varia

Proposed changelog entry for your changes:

none

matticbot · 2020-04-21T17:47:15Z

Caution: This PR has changes that must be merged to WordPress.com
Hello WunderBart! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D42226-code before merging this PR. Thank you!
This revision will be updated with each commit to this PR

WunderBart · 2020-04-23T11:01:28Z

Currently, on front-end, the Player doesn’t render cover art, description and doesn’t pick up custom colors. This is all going to be fixed when #15515 lands 🙌

- Create link class to be sure all links look the same, - Make all links look the same, - Use correct default color for description instead of "$dark-gray-500", - Clean up and make colors styles more consistent and easy to follow

Varia also uses the `has-background` class which bumps its selectors specificity unintendedly. This makes our anchor styles more specific.

jetpackbot · 2020-04-23T14:22:49Z

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Scheduled Jetpack release: May 5, 2020.
Scheduled code freeze: April 28, 2020

Generated by 🚫 dangerJS against 66b4af6

Should not be the same as the floating tooltip

jeryj

Looks a lot cleaner overall! Nice work! 🙌 I'm approving for now, but waiting to tag Jetpack Crew until the WPcom patch applies cleanly.

I tested with various Twenty * themes, Varia, Varia-based child themes, and Global styles.

jeryj · 2020-04-23T17:05:21Z

 			 */ }
 			{ !! ( showEpisodeDescription && track && track.description ) && (
-				<div
+				<p


Good catch to change this to a <p>. Testing looks good!

jeryj · 2020-04-23T17:07:12Z

+$gutter-s: 10px;
+$gutter-m: 15px;
+$gutter-l: 24px;


Glad this change worked out nicely!

jeryj · 2020-04-23T18:30:14Z

 			}
 		}
+
+		a.jetpack-podcast-player__link {


I like how you consolidated this for the title and track links into this class.

jeherve

LGTM! 🚢

WunderBart · 2020-04-24T16:30:14Z

r206420-wpcom

WunderBart added [Status] In Progress [Block] Podcast Player labels Apr 21, 2020

WunderBart self-assigned this Apr 21, 2020

matticbot added the Touches WP.com Files label Apr 21, 2020

WunderBart marked this pull request as ready for review April 22, 2020 14:06

WunderBart added [Status] Needs Team Review Obsolete. Use Needs Review instead. and removed [Status] In Progress labels Apr 22, 2020

WunderBart requested a review from jeryj April 22, 2020 14:06

WunderBart added [Status] In Progress and removed [Status] Needs Team Review Obsolete. Use Needs Review instead. labels Apr 22, 2020

WunderBart added [Status] Needs Team Review Obsolete. Use Needs Review instead. and removed [Status] In Progress labels Apr 23, 2020

WunderBart and others added 16 commits April 23, 2020 14:13

Clean up duplicated CSS variables

da36b38

Don't use variable for ME background color

ea5fdf8

Use editor variables for standard font sizes

308c13b

Rename a few variables for clarity

fe61f2a

Move state classes comment under proper class

aa9134b

Move block background-color where it belongs

9ba33ea

Move comment over related code

8bff114

Improve colors styling

1b0b047

- Create link class to be sure all links look the same, - Make all links look the same, - Use correct default color for description instead of "$dark-gray-500", - Clean up and make colors styles more consistent and easy to follow

Spread some shorthands for clarity and lint styles.scss

a2c109b

Reorganized .mejs wrapper classes for easier reading

65ff357

Move link class for specificity bump

174c2e2

Varia also uses the `has-background` class which bumps its selectors specificity unintendedly. This makes our anchor styles more specific.

Remove block border

5d5b90b

Update header markup

3da2053

Add missing class to track link

0e6698c

Correct track padding for no-js markup

fd729e9

Move no-js styling to the bottom of the sheet

a3edfb9

WunderBart force-pushed the update/podcast-player-cleanup-colors branch from bd4893b to dbfb2d6 Compare April 23, 2020 14:16

matticbot added Touches WP.com Files and removed Touches WP.com Files labels Apr 23, 2020

jeryj reviewed Apr 23, 2020

View reviewed changes

Comment thread extensions/blocks/podcast-player/style.scss Outdated

WunderBart added 2 commits April 23, 2020 16:27

Use gutter-m instead of fixed value

f6a1839

Use provided var for description line-height

444590c

matticbot added Touches WP.com Files and removed Touches WP.com Files labels Apr 23, 2020

Wrap description text with <p>

037cc6e

jeryj reviewed Apr 23, 2020

View reviewed changes

Comment thread extensions/blocks/podcast-player/style.scss

jeryj reviewed Apr 23, 2020

View reviewed changes

Comment thread extensions/blocks/podcast-player/style.scss

WunderBart and others added 3 commits April 23, 2020 17:01

Do not add extra padding to the top of the list

21cf495

Fix ME.js time element color

c0fb34b

Should not be the same as the floating tooltip

Removed unnecessary aria-live in SSR podcast header

66b4af6

jeryj approved these changes Apr 23, 2020

View reviewed changes

jeryj removed the [Status] Needs Team Review Obsolete. Use Needs Review instead. label Apr 23, 2020

jeherve added this to the 8.5 milestone Apr 24, 2020

jeherve added the [Status] Needs Review This PR is ready for review. label Apr 24, 2020

jeherve approved these changes Apr 24, 2020

View reviewed changes

jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels Apr 24, 2020

WunderBart merged commit e86c2db into master Apr 24, 2020

matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Apr 24, 2020

jeherve removed the [Status] Needs Changelog label Apr 24, 2020

WunderBart deleted the update/podcast-player-cleanup-colors branch April 27, 2020 11:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Podcast Player: Clean-up styles & fix few remaining issues#15512

Podcast Player: Clean-up styles & fix few remaining issues#15512
WunderBart merged 25 commits into
masterfrom
update/podcast-player-cleanup-colors

WunderBart commented Apr 21, 2020 •

edited

Loading

Uh oh!

matticbot commented Apr 21, 2020

Uh oh!

WunderBart commented Apr 23, 2020

Uh oh!

jetpackbot commented Apr 23, 2020 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jeryj left a comment •

edited

Loading

Uh oh!

jeryj Apr 23, 2020

Uh oh!

jeryj Apr 23, 2020

Uh oh!

jeryj Apr 23, 2020

Uh oh!

jeherve left a comment

Uh oh!

WunderBart commented Apr 24, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

WunderBart commented Apr 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes proposed in this Pull Request:

Testing instructions:

Proposed changelog entry for your changes:

Uh oh!

matticbot commented Apr 21, 2020

Uh oh!

WunderBart commented Apr 23, 2020

Uh oh!

jetpackbot commented Apr 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jeryj left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeryj Apr 23, 2020

Choose a reason for hiding this comment

Uh oh!

jeryj Apr 23, 2020

Choose a reason for hiding this comment

Uh oh!

jeryj Apr 23, 2020

Choose a reason for hiding this comment

Uh oh!

jeherve left a comment

Choose a reason for hiding this comment

Uh oh!

WunderBart commented Apr 24, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

WunderBart commented Apr 21, 2020 •

edited

Loading

jetpackbot commented Apr 23, 2020 •

edited

Loading

jeryj left a comment •

edited

Loading