WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 21 months ago

#37596 closed enhancement (invalid)

Update Press This' `normalize.css` 3rd party external library

Reported by: netweb Owned by:
Milestone: Priority: normal
Severity: normal Version: 4.2
Component: Press This Keywords: needs-testing
Focuses: Cc:

Description

Press This includes the Normalize library as part of press-this.css

Press This currently uses v3.0.0 of Normalize and the most recent version at time of writing is v4.2.0 which now includes support for Microsoft's Edge browser and various updates and tweaks for Chrome, Firefox, and Safari

Prior to Press This v2 being merged into core the original source file scss/_utilities/_normalize.scss included the Normalize library's comments and license, these were removed in preparation for the merge into core.

There have been 3 changes to Normalize library section of press-this.css since the library was introduced:

The above changes should be tested with the latest Normalize (4.2) and merged into core with the original source comments and license.

Also adding a reference to the library in the "External Libraries" credits at wp-admin/credits.php

Attachments (6)

37596.diff (9.1 KB) - added by wonderboymusic 3 years ago.
37596.1.diff (9.3 KB) - added by netweb 3 years ago.
37596.2.diff (13.0 KB) - added by desrosj 3 years ago.
37596.3.diff (13.8 KB) - added by desrosj 3 years ago.
Updated patch including Normalize 5.0.0, in case we do decide to move forward with this method.
no normalize-desktop.png (123.2 KB) - added by desrosj 3 years ago.
Press This without Normalize dependency
no normalize-mobile.png (75.2 KB) - added by desrosj 3 years ago.
No normalize dependency, mobile.

Download all attachments as: .zip

Change History (23)

#1 @kraftbj
3 years ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 4.7

Thanks for catching this. I concur.

@wonderboymusic
3 years ago

#2 @wonderboymusic
3 years ago

  • Keywords has-patch added; needs-patch removed

@netweb
3 years ago

#3 @netweb
3 years ago

Also adding a reference to the library in the "External Libraries" credits at wp-admin/credits.php

We should add a credit yes? I've created #meta1932 with a patch for this.

In 37596.1.diff I've added section header comments per the CSS Coding Standards.

#4 follow-up: @jorbin
3 years ago

What about moving normalize into its own file and then having press-this.css include it? Seems like that will make it easier to keep this up to date going forward.

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


3 years ago

#6 in reply to: ↑ 4 @desrosj
3 years ago

Replying to jorbin:

What about moving normalize into its own file and then having press-this.css include it? Seems like that will make it easier to keep this up to date going forward.

Do you see making Normalize available through the wp_enqueue_style()? If so, we can just add Normalize as a stylesheet dependency to press-this.css, and then theme/plugin authors could load it if desired.

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


3 years ago

@desrosj
3 years ago

#8 @desrosj
3 years ago

  • Keywords needs-testing added

Added Normalize as a separate CSS file. Press This loads it via dependency. Themes can now enqueue Normalize to prevent loading it twice.

#9 @ocean90
3 years ago

Are there any issues with the current version which is used by Press This? I'm wondering if Press This requires all bundled normalizations or if we could use a trimmed version instead.

#10 @jorbin
3 years ago

@ocean90 I would really rather we didn't edit third party libraries without good cause. This is ~200 lines of css outside of comments and whitespace.

#11 @ocean90
3 years ago

@jorbin Sorry, I wasn't clear enough. I'm not talking about editing the library. I was thinking about extending our existing selectors with reset rules, if necessary, and remove Normalize as a dependency.
I just removed the Normalize rules from press-this.css and it doesn't look that bad.

#12 @azaozz
3 years ago

Agree with @ocean90. Not sure we need yet another dependency that is used only in one place for one thing. CSS normalization used to be a "big thing" long ago when IE6 was around. There was even "global" normalization for all of the admin CSS. That was removed years ago, don't see a compelling reason to add back something similar.

Would be good to hear what @michaelarestad (who made that CSS) thinks about updating the normalization. As far as I see it's not needed. Seems it was used as a base/good place to start from as the Press This CSS is separate from the rest of wp-admin CSS.

#13 @netweb
3 years ago

  • Keywords needs-refresh added; has-patch removed

FYI: Normalize has been updated to v5, so if this is to go in, needs-refresh.

https://github.com/necolas/normalize.css/blob/master/CHANGELOG.md#500-october-3-2016

@desrosj
3 years ago

Updated patch including Normalize 5.0.0, in case we do decide to move forward with this method.

@desrosj
3 years ago

Press This without Normalize dependency

@desrosj
3 years ago

No normalize dependency, mobile.

#14 @desrosj
3 years ago

  • Keywords needs-refresh removed

I updated the patch to include the 5.0.0 version of Normalize. I also attached some screenshots to show issues when not loading Normalize as a dependency. There are not that many. After first glance the following needs work:

  1. Publish buttons
  2. Mobile tag icon top right used for opening the slide out menu
  3. Margin around the outside of the page.
  4. Media items are larger in the import area above the editor.

I do not have the ability to test some older browsers, but for current versions the changes needed to eliminate the dependency are minimal.

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


3 years ago

#16 @ocean90
3 years ago

  • Milestone changed from 4.7 to Future Release
  • Type changed from defect (bug) to enhancement

See comment:12.

#17 @kraftbj
21 months ago

  • Milestone Future Release deleted
  • Resolution set to invalid
  • Status changed from new to closed

Will continue the conversation at https://github.com/WordPress/press-this/issues/8 with PT being moved into a plugin and out of Core.

Note: See TracTickets for help on using tickets.