WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

#38389 closed defect (bug) (fixed)

Twenty Seventeen: Refresh color patterns for changes in style.css

Reported by: melchoyce Owned by: davidakennedy
Milestone: 4.7 Priority: normal
Severity: normal Version:
Component: Bundled Theme Keywords: has-patch needs-testing
Focuses: Cc:

Description

Interior page headers:
https://cloud.githubusercontent.com/assets/2846578/19464764/ece64f20-94cc-11e6-8aa5-d47229dad261.png

Galleries:
https://cloud.githubusercontent.com/assets/2846578/19464761/ecdb6b14-94cc-11e6-88d0-a6d5dd08963c.png

Widget image (linked):
https://cloud.githubusercontent.com/assets/2846578/19464763/ece52a0a-94cc-11e6-98e6-7243cb7e5d93.png

Tables only just got merged:
https://cloud.githubusercontent.com/assets/2846578/19464762/ece47a7e-94cc-11e6-8c7e-be83a04d9ab4.png

Attachments (3)

38389.diff (17.2 KB) - added by celloexpressions 4 years ago.
Update color-patterns.php for all commits to style.css between October 4th and core merge.
38389.1.diff (41.0 KB) - added by laurelfulford 4 years ago.
38389.2.diff (41.0 KB) - added by laurelfulford 4 years ago.

Download all attachments as: .zip

Change History (19)

#1 @karmatosed
4 years ago

  • Owner set to celloexpressions
  • Status changed from new to assigned

Assigning to celloexpressions as said on GitHub would tackle this.

#2 @celloexpressions
4 years ago

  • Summary changed from Twenty Seventeen: Colors: Header background, gallery images and captions, widget images, tables missing to Twenty Seventeen: Refresh color patterns for changes in style.css

Updating at least the ticket summary to reflect the approach here. I'm going to go through all of the changes made to style.css after October 4th and sync them to both color-patterns.php and colors-dark.css. The colors files were forked from the stylesheet on that date, and organized by color.

@davidakennedy please ensure that every patch and commit to style.css moving forward also makes the corresponding edits to the colors file. Any selector change with any colors in it, and definitely any changes to color-related properties (including border color) needs to be made in both places. I'll take care of everything that happened on GitHub, but the core merge is a good line for when color changes started being handled as changes are made.

#3 @davidakennedy
4 years ago

@celloexpressions Thank you and agreed! I'll keep an eye out for anything color related.

@celloexpressions
4 years ago

Update color-patterns.php for all commits to style.css between October 4th and core merge.

#4 @celloexpressions
4 years ago

  • Owner changed from celloexpressions to davidakennedy
  • Status changed from assigned to reviewing

38389.diff re-syncs the color-patterns.php template with all of the changes between October 4th and core merge that were committed to style.css. Unfortunately, the way git works, it's extremely tedious to do this since there are cross-merges across branches and a lot of iterations to diffs to fix coding standards, etc. in the history. This is pretty close, but I imagine there will be a few more bugs that spring up.

The dark scheme should probably be entirely remade based on the revised color patterns file. If someone can list out how each color should be mapped from the originals again that would help, then there are some additional color inversions that may be needed on top of it. I didn't apply custom colors to the dark playlist styles, and I suppose that if someone is using the dark scheme they'd select the dark playlist style anyway, so probably don't need to change anything there.

#5 @davidakennedy
4 years ago

  • Keywords has-patch needs-testing added; needs-patch removed

#6 @davidakennedy
4 years ago

#38385 was marked as a duplicate.

#7 @davidakennedy
4 years ago

Thanks @celloexpressions! This looks pretty good. The biggest issue I see is that the color schemes do not have the new link styles.

Those were brought in here: https://github.com/WordPress/twentyseventeen/pull/428

#8 @celloexpressions
4 years ago

They should have the new link styles, but there may be some bugs there. You can see that a big chunk of the diff moves link selectors from the background color to the black text color and the hover state override is also there. But it was hard for me to figure out exactly what might be missing, so if someone that worked on that change could make adjustments to 38389.diff as needed that would be great.

#9 @davidakennedy
4 years ago

Thanks @celloexpressions! @laurelfulford is going to take a stab at reviewing and updating anything.

#10 @laurelfulford
4 years ago

In 38389.1.diff I updated the styles to include the new link styles, in both the colors-dark.css and color-patterns.php. Hopefully that got everything - just let me know if anything isn't displaying as expected!

#11 @karmatosed
4 years ago

#38478 was marked as a duplicate.

#12 @celloexpressions
4 years ago

It's hard to tell in the diff, but let's make sure that all of the color patterns remain ordered from light to dark or vice-versa in both of these files. And maybe double-check that there's a comment at the top indicating that, I think there may be one there already.

I unfortunately have to work on a couple of higher priority things so don't have time to test at the moment, but we can continue iterating and it's probably better to get the bulk of the fixes in sooner rather than later. We'll catch most of the lingering issues in beta.

#13 @karmatosed
4 years ago

I'm still not seeing this working on dark colors this is what I see with the latest patch unfortunately.

https://cldup.com/2er5kDLVA0.png

I did a hard refresh and re-picked colors just incase.

#14 @davidakennedy
4 years ago

@laurelfulford This looks good. Almost ready for commit. One issue I see:

.project-terms is referenced in the style.css and is likely left over from Lodestar fork. Can you all traces? It's not needed.

#15 @laurelfulford
4 years ago

Updated the changes in 38389.2.diff - includes fixing one out-of-order colour, adding a comment to each file about the colours being in order, and removing the unneeded .project-terms selector from the style.css and both colour files.

Just let me know if there's anything else!

#16 @davidakennedy
4 years ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 38918:

Twenty Seventeen: Refresh color patterns for changes in style.css

This brings the custom colors CSS in line with the stylesheet. Some selectors were missing since many changes occurred in style.css a few days before the original merge to Core.

Props celloexpressions, laurelfulford.

Fixes #38389.

Note: See TracTickets for help on using tickets.