Skip to content
This repository was archived by the owner on Dec 1, 2019. It is now read-only.

See #284 #351

Merged
merged 74 commits into from
Sep 21, 2019
Merged

See #284 #351

merged 74 commits into from
Sep 21, 2019

Conversation

aristath
Copy link
Member

@aristath aristath commented Sep 18, 2019

This addresses #284. Not to be merged yet, this is a proof of concept. What it does: (see screencast below)

demo2

The implementation here replaces the PHP class that was already added in #249 and that is no longer needed, if this get merged (after we tweak it) then the other class can be removed.

I'll leave comments in the code so we can review things and decide what to do.

For now I only did this for the content area but the code can handle as many areas as we want, so it can different for the header, footer, overlays etc.

@ianbelanger79
Copy link
Contributor

This is awesome!!! Can't wait to merge the final product

Copy link
Member Author

@aristath aristath left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The filenames I chose will have to be changed too, I deliberately chose the wrong filenames to avoid conflicts with any other PRs that may get merged before this one.
The files should be called customize-preview.js and customize-controls.js.
If we decide to merge this PR then we can move the contents of the wrong filenames to the correct ones (or just rename them if they don't exist 'till then).

@pattonwebz pattonwebz added the WIP Work In Progress: DO NOT MERGE label Sep 18, 2019
@pattonwebz
Copy link
Member

This really is a very cool control to play with. We will need to come up with some good inline help text to accompany it so that users understand how it works but I like this a lot.

We can look at doing a little code cleanup and rearranging once we have had a few people test this.

Here is another gif example of it in action going from one extreme of the color selector to the other

Peek 2019-09-18 21-44

@aristath
Copy link
Member Author

aristath commented Sep 18, 2019

Oh, and an additional note. If this gets merged we should change the grays. Instead of doing this:

.post-meta {
    color: #6d6d6d;
}

we should be doing this:

.post-meta {
    color: currentColor;
    opacity: .7; /* or whatever value produces the same result. */
}

That way no colors will be hardcoded and instead they'll also change along with the textcolor.

@celloexpressions
Copy link

Great first pass - I'll look through the code and comment over the next day or so. For grays, we need to consider: #dcd7ca - borders and #6d6d6d - secondary text, as I noted in #284. All text, borders, icons, etc. should be updated as needed. We will likely need to reduce the relative difference between primary and secondary colors to maintain some contrast with the background color (maybe target 3:1 minimum instead of 4.5:1?).

The complete CSS "template" for colors should be built by starting with all of style.css and stripping it down to just the colors. This needs to happen once style.css is as stable as possible; all future changes to selectors with colors will also need to be synced to the JS template.

The template is currently structured with arrays of selectors and other information. We may want to consider formatting it to look more like regular CSS for ease of updating. This could look similar to JS-templated customize controls (example).

@aristath
Copy link
Member Author

If the array of elements is kept in PHP we only need to maintain it in 1 place. It can be passed-on the the JS file as a var

@acosmin
Copy link
Contributor

acosmin commented Sep 19, 2019

Looks like a winner, I'm gonna go ahead and close my PRs.

For the generated CSS, maybe you guys can do something like #304 and make them filterable.

@aristath
Copy link
Member Author

@celloexpressions thank you for all the suggestions, they really made this implementation a lot better, I wouldn't have thought of some of these things without you. 🥇

@ianbelanger79
Copy link
Contributor

@aristath We just merged eslint into master and there are now a few errors/warning that need to be fixed, after that we will merge this

@aristath
Copy link
Member Author

@ianbelanger79 I was already working on fixing them. Pushed a commit just now, should fix any and all issues. We'll know in a few minutes 👍

@ianbelanger79
Copy link
Contributor

Awesome, after travis passes, this will be getting merged.

Copy link
Contributor

@ianbelanger79 ianbelanger79 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is awesome!! Time to merge it

@ianbelanger79 ianbelanger79 merged commit 049e8fe into WordPress:master Sep 21, 2019
@aristath
Copy link
Member Author

aristath commented Sep 21, 2019

Yay!!!! ❤️ 🎉
Thank you for merging this

@ianbelanger79
Copy link
Contributor

Thanks for all of your work on this @aristath and @celloexpressions. This is going to make Twenty Twenty a much better and more accessible theme.

@celloexpressions
Copy link

Thanks for all of your hard work on this @aristath! This will be a great feature to use. I'll take a look through what was merged this weekend an make any comments on #284 or a new issue for any adjustments to make during beta.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
WIP Work In Progress: DO NOT MERGE
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants