Make WordPress Core

Opened 7 years ago

Closed 7 years ago

Last modified 4 years ago

#40745 closed task (blessed) (fixed)

Bundled Themes: Add styles for wysiwyg Text and Media widgets

Reported by: soean's profile Soean Owned by: obenland's profile obenland
Milestone: 4.8 Priority: normal
Severity: blocker Version: 4.8
Component: Bundled Theme Keywords: has-patch ui-feedback has-screenshots needs-testing
Focuses: ui Cc:

Description

In #35760 the Text widget was extended with TinyMCE. The Text widget supports now bold, italic, lists and links. The default themes should support the styles for the html elements.

Attachments (23)

40745.diff (3.7 KB) - added by Soean 7 years ago.
Adds styles for Text widget.
40745.2.diff (3.7 KB) - added by Soean 7 years ago.
40745.3.diff (3.7 KB) - added by Soean 7 years ago.
40745.4.diff (5.8 KB) - added by obenland 7 years ago.
twentyten-media-widgets.PNG (137.8 KB) - added by celloexpressions 7 years ago.
twentyeleven-media-widgets.PNG (116.1 KB) - added by celloexpressions 7 years ago.
twentytwelve-media-widgets.PNG (190.5 KB) - added by celloexpressions 7 years ago.
twentythirteen-media-widgets.PNG (148.4 KB) - added by celloexpressions 7 years ago.
twentyfourteen-media-widgets.PNG (210.4 KB) - added by celloexpressions 7 years ago.
twentyfifteen-media-widgets.PNG (197.3 KB) - added by celloexpressions 7 years ago.
twentysixteen-media-widgets.PNG (225.6 KB) - added by celloexpressions 7 years ago.
twentyseventeen-media-widgets.PNG (238.9 KB) - added by celloexpressions 7 years ago.
40745.5.diff (14.1 KB) - added by obenland 7 years ago.
twentyten.png (323.7 KB) - added by obenland 7 years ago.
twentyeleven.png (251.1 KB) - added by obenland 7 years ago.
twentytwelve.png (183.3 KB) - added by obenland 7 years ago.
twentythirteen.png (369.8 KB) - added by obenland 7 years ago.
twentyfourteen.png (255.8 KB) - added by obenland 7 years ago.
twentyfifteen.png (160.6 KB) - added by obenland 7 years ago.
twentyseventeen.png (315.2 KB) - added by obenland 7 years ago.
twentythirteen-audio-video-widgets-footer.PNG (45.0 KB) - added by celloexpressions 7 years ago.
Twenty Thirteen: should we use a different background color on media elements in the footer widget area? It matches the footer background currently.
twentyfourteen-audio-video-widgets-content.PNG (379.8 KB) - added by celloexpressions 7 years ago.
Twenty Fourteen: completed fix with matching iconography and colors on media players in content and the content sidebar widget area (patch forthcoming).
40745.6.diff (17.1 KB) - added by celloexpressions 7 years ago.
Fix media player icons for Twenty Fourteen, amending 40725.6.diff (also fixes spacing issue likely caused by a past MediaElement.js update).

Change History (51)

@Soean
7 years ago

Adds styles for Text widget.

@Soean
7 years ago

#1 @obenland
7 years ago

  • Milestone changed from Awaiting Review to 4.8

@Soean
7 years ago

#2 @Soean
7 years ago

  • Keywords has-patch added

@obenland
7 years ago

#3 @obenland
7 years ago

  • Keywords needs-screenshots ui-feedback added

@Soean Could you give 40745.4.diff a try and see how you like it?
We would also need before/after pictures for each theme, for folks to get an idea of the changes.

Thanks!

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


7 years ago

#5 @celloexpressions
7 years ago

  • Component changed from Themes to Bundled Theme
  • Focuses ui added
  • Summary changed from Themes: Add styles for wysiwyg Text widget to Bundled Themes: Add styles for wysiwyg Text and Media widgets

Adding a series of "before" screenshots for each bundled theme below, and also including media widgets here as they need some related adjustments. That way we can streamline the cross-theme testing and documentation for both widget types.

#6 @celloexpressions
7 years ago

  • Severity changed from normal to blocker

After evaluating all eight bundled themes, there are some critical issues that need to be fixed. Most notably is list styling - for many themes, there are no bullet points, and the way the editor converts things to ul automatically, this could lead to a lot of frustration and confusing/unexpected results. Additionally, I found several other issues listed below. The scope of my testing was limited generally to each theme's primary widget area.

I don't have time to test them all with the patch, but it looks like 40745.4.diff addresses many of the issues. The MediaElement.js inconsistencies in Twenty Thirteen and Twenty Fourteen still need to be addressed there. Once the issues listed below are verified as fixed, someone should do a pass on the general design to make other less obvious improvements. Where possible, the original theme designers should be consulted.

Based on the scope of issues, and over half of the bundled themes requiring at least some adjustments, there needs to be a dev note on make/core to discuss updating themes to support the new widgets, with some of the common issues (list styling with bullets, list spacing, MediaElements, image spacing, etc.). Time should also be allotted and effort made with the Theme Review Team to do outreach to theme authors to give them time to evaluate and update their themes.

Summary of definite issues that need to be fixed (based on current trunk):

Twenty Ten:

  • No issues

Twenty Eleven:

  • Image widget with caption padding issues
  • Unordered vs. ordered list alignment issues

Twenty Twelve:

  • No issues

Twenty Thirteen:

  • Ordered lists missing ordering
  • Unordered lists missing bullets
  • List spacing is very odd
  • MediaElement.js players are missing custom theme coloring and do not match media players in post content

Twenty Fourteen:

  • Ordered lists missing ordering
  • Unordered lists missing bullets
  • MediaElement.js players are missing custom theme coloring and do not match media players in post content
  • MediaElement.js players are missing custom theme-specified icons (via Genericons, and associated icon recoloring) and do not match media players in post content. Screenshot doesn't show it, but the video player interactions are particularly different.

Twenty Fifteen:

  • Inadequate color contrast in image widget caption (and possibly also in text widget - needs accessibility team verification)

Twenty Sixteen:

  • No issues

Twenty Seventeen:

  • Unordered lists missing bullets
  • List spacing is very odd
  • List numbers hanging into margin come very close to adjacent content area (not visible in this particular screenshot)

This ticket was mentioned in Slack in #core-customize by obenland. View the logs.


7 years ago

This ticket was mentioned in Slack in #themereview by joyously. View the logs.


7 years ago

@obenland
7 years ago

@obenland
7 years ago

@obenland
7 years ago

@obenland
7 years ago

#9 @obenland
7 years ago

  • Keywords has-screenshots added; needs-screenshots removed

40745.5.diff improves styles. I added before/after screenshots of the changes, please see above.

@celloexpressions I duplicated the custom mediaelement styles for widgets, would you mind testing those again?

This ticket was mentioned in Slack in #core by obenland. View the logs.


7 years ago

#11 @obenland
7 years ago

  • Type changed from defect (bug) to task (blessed)

#12 @ocean90
7 years ago

40745.5.diff looks good to me. Worth to mention that we also support the formatting shortcuts in the editor so elements like headings, block quotes, horizontal lines and inline code are also available. But those are also looking good.

@celloexpressions
7 years ago

Twenty Thirteen: should we use a different background color on media elements in the footer widget area? It matches the footer background currently.

@celloexpressions
7 years ago

Twenty Fourteen: completed fix with matching iconography and colors on media players in content and the content sidebar widget area (patch forthcoming).

@celloexpressions
7 years ago

Fix media player icons for Twenty Fourteen, amending 40725.6.diff (also fixes spacing issue likely caused by a past MediaElement.js update).

#13 @celloexpressions
7 years ago

  • Keywords needs-testing added

Did a couple quick spot checks. Minor issues:

  • Twenty Seventeen: appears to have more space before a list than after the list, which looks a bit off. Possibly also the same issue in Twenty Fourteen.
  • Twenty Thirteen: we may want to select one of the other theme colors for the background color of the MediaElement players when they're used in the primary (footer) widget area, as the background of that widget area matches the color used for players in the content.

I also made some additional fixes for MediaElement.js player in Twenty Fourteen in 40745.6.diff, which theoretically includes everything in 40745.5.diff but please double-check.

Let's try to round up several people to do some testing on each of the themes - these styles will be used on a lot of sites and we need to make sure they're up to the quality standards of bundled themes. With different widget areas and configurations, we'll discover a few minor tweaks that we can make to improve things.

This ticket was mentioned in Slack in #core by obenland. View the logs.


7 years ago

#15 @Joen
7 years ago

Twenty Thirteen: should we use a different background color on media elements in the footer widget area? It matches the footer background currently.

I don't think that's critically necessary. But if need be, we can use #1b1b19 or even just plain black. I think that'd be good.

#16 @karmatosed
7 years ago

I have tested every theme using the latest patch. Here are the outstanding issues I think we need to care for.

  1. Twenty Fourteen:

This we may put down to a design decision, but because of the higher break point with less padding, you get really bid videos and audio:

https://cldup.com/fVyueEq8e9.png

  1. Twenty Fourteen:

I am unsure the dark audio on dark sidebar works well:

https://cldup.com/oXDJztPTaT.png

Twenty Thirteen has this too, but not critical.


Here is a visual record of each theme:

Twenty-seventeen:
video, image, audio widget:
https://cldup.com/nwgDrpJf3a.png

text widget:
https://cldup.com/DOgr9XySiw.png

mobile:
https://cldup.com/FsJ6LdFd8H.png

Twenty Sixteen:

video, image, audio widget:
https://cldup.com/cjiQz0xE-F.png

text widget:
https://cldup.com/PBt9Wk4ZJU.png

mobile:
https://cldup.com/cByEZmX91B.png

Twenty Fifteen:

video, image, audio widget:
https://cldup.com/oIH91eIqLi.png

text widget:
https://cldup.com/oMRCtqpoBY.png

mobile:
https://cldup.com/JSl4TCJQwu.png

Twenty Fourteen:

video, image, audio widget:
https://cldup.com/oXDJztPTaT.png

text widget:
https://cldup.com/MfW2JUjq8Q.png

mobile:
https://cldup.com/jGfE9VGwS2.png

Twenty Thirteen:

video, image, audio widget:
https://cldup.com/BycmH4IfTb.png

text widget:
https://cldup.com/scqbFxmiBz.png

mobile:
https://cldup.com/cDqYZ7M6Wc.png

Twenty Twelve:

video, image, audio widget:

https://cldup.com/5YNaWtRwWC.png

text widget:
https://cldup.com/tjmMAzR1N3.png

mobile:
https://cldup.com/1eiZox7RB2.png

Twenty Eleven:

video, image, audio widget:
https://cldup.com/ebAAJE6egs.png

text widget:
https://cldup.com/E6_guaNLnC.png

Twenty Ten:

video, image, audio widget:
https://cldup.com/UOvYxv_yvn.png

text widget:
https://cldup.com/sTvm63QxvS.png

Last edited 7 years ago by karmatosed (previous) (diff)

This ticket was mentioned in Slack in #core by karmatosed. View the logs.


7 years ago

#18 @karmatosed
7 years ago

With regards to the dark/on dark issue in Twenty Fourteen, what you get for the dark color on Twenty Fifteen is this:

https://cldup.com/V49QKTzqd2.png

I would suggest if we do it, then this is a good fix.

#19 @karmatosed
7 years ago

I am testing the color variations in themes also and notice for example Twenty Seventeen's dark:

https://cldup.com/yTyDc9ZEm2.png

As several themes have this we may want to just put it down to design decisions and not fix.

#20 @karmatosed
7 years ago

I can confirm no new issues when testing the default theme color variations.

This ticket was mentioned in Slack in #core by desrosj. View the logs.


7 years ago

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


7 years ago

#23 @jbpaul17
7 years ago

  • Keywords needs-dev-note added

#24 @obenland
7 years ago

  • Owner set to obenland
  • Resolution set to fixed
  • Status changed from new to closed

In 40839:

Default themes: Improve styles for 4.8 widgets

Mostly adds styles for lists and mediaelement.js instances within widgets.
Adds size classname to image widget so themes can customize their display.

Props Soean, obenland, celloexpressions, ocean90, karmatosed.
Fixes #40745.

This ticket was mentioned in Slack in #core by davidakennedy. View the logs.


7 years ago

#26 @jorbin
7 years ago

In 40883:

Bundled Themes - Bump themes in preparation for 4.8

Change version numbers in stylesheets, fix typo in Twenty Fourteen readme and update copyright dates for themes. Twentysixteen wasn't updated this cycle.

See #40745 for changes that helped push this along.
Fixes #40905.
Props davidakennedy, mrahmadawais, maedahbatool.

#27 @jorbin
7 years ago

In 40884:

Bundled Themes - Bump themes in preparation for 4.8

Backports [40883] from trunk to 4.8

Change version numbers in stylesheets, fix typo in Twenty Fourteen readme and update copyright dates for themes. Twentysixteen wasn't updated this cycle.

See #40745 for changes that helped push this along.
Fixes #40905.
Props davidakennedy, mrahmadawais, maedahbatool.

#28 @desrosj
4 years ago

  • Keywords needs-dev-note removed

As far as I can tell, this change did not receive a developer note. Removing the needs-dev-note keyword.

Note: See TracTickets for help on using tickets.