Skip to content
This repository has been archived by the owner on Jun 18, 2020. It is now read-only.

Varya: try adding gradient preset #95

Merged
merged 10 commits into from
Apr 24, 2020
Merged

Varya: try adding gradient preset #95

merged 10 commits into from
Apr 24, 2020

Conversation

jffng
Copy link
Contributor

@jffng jffng commented Apr 16, 2020

This PR adds a single gradient preset (#93) implementing the diagonal cutting background:

Screen Shot 2020-04-16 at 5 11 04 PM

A few considerations:

  • Where should something like this live in Varya? For now I threw it into a new file under sass/base called _presets.scss cc @allancole
  • Child themes can easily take advantage of this preset by quickly changing the colors via a CSS variable change. However, the implementation of custom gradient presets require that you declare them in the functions.php file, so the editor can show a preview in the block settings. This is just a bit more maintenance to keep it in sync, but I'm noting this so that our child theme build tool can account for this it, unless there's another approach.
  • There's some slight aliasing happening along the diagonal. Is this acceptable?

@jffng jffng requested a review from kjellr April 16, 2020 20:55
@jffng jffng self-assigned this Apr 16, 2020
@jffng jffng marked this pull request as draft April 16, 2020 20:56
@jffng
Copy link
Contributor Author

jffng commented Apr 16, 2020

Ah, I'm an idiot and didn't finish reading the docs 😅 .

Themes are responsible for creating the classes that apply the gradients.

Pushing that shortly.

@jffng jffng marked this pull request as ready for review April 16, 2020 21:21
@jffng jffng requested a review from allancole April 16, 2020 21:21
@jffng jffng added the enhancement New feature or request label Apr 16, 2020
@kjellr kjellr added this to the Varya: 1.0 Release milestone Apr 16, 2020
@allancole
Copy link
Collaborator

Where should something like this live in Varya? For now I threw it into a new file under sass/base called _presets.scss cc @allancole

Feels like that should go in /varya/assets/sass/blocks/utilities/_presets.scss

There's some slight aliasing happening along the diagonal. Is this acceptable?

Hmmm, is that a common issue with diagonals?

@kjellr
Copy link
Contributor

kjellr commented Apr 16, 2020

There's some slight aliasing happening along the diagonal. Is this acceptable?

Mind adding a screenshot?

@jffng
Copy link
Contributor Author

jffng commented Apr 17, 2020

Aliasing along the diagonal screenshot:

Screen Shot 2020-04-17 at 9 39 16 AM

Copy link
Collaborator

@allancole allancole left a comment

Choose a reason for hiding this comment

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

I don’t mind the aliasing at all. It’s too subtle to be a deal breaker for me. Once the file structure stuff is sorted out I say LGTM!

@kjellr
Copy link
Contributor

kjellr commented Apr 21, 2020

I wonder if @jasmussen or @MichaelArestad happens to know anything about smoothing out that messy gradient aliasing? It feels like there's gotta be some sort of hack out there, but I haven't found it yet...

Screen Shot 2020-04-21 at 1 23 37 PM

I tried applying slight blur via something like backdrop-filter, but it gets too blurry.

@jasmussen
Copy link
Member

I'm mobile at the moment so I haven't been able to test. But looking at the code it appears one half if the diagonal split stops at 50%, and the next starts at 50%. The aliasing between those two colors might simply by the entire color range being compressed into a singularity.

So my first instinct would be to try and end the first half of the gradient at 49.9%, and start the second half of the gradient at 50.1%, and seeing if allowing a tiny bit of gradation could actually translate into a form of antialiasing. And if yes, I'd massage those percentages to be as close as possible.

@kjellr
Copy link
Contributor

kjellr commented Apr 21, 2020

So my first instinct would be to try and end the first half of the gradient at 49.9%, and start the second half of the gradient at 50.1%, and seeing if allowing a tiny bit of gradation could actually translate into a form of antialiasing. And if yes, I'd massage those percentages to be as close as possible.

Good thought! Unfortunately, I just tried it out but it appears to have no effect. 😞

I tried also adding a middle 1% of an average color (the average of those two), but it didn't seem to fix anything either.

@jasmussen
Copy link
Member

Thanks for trying. Bummer.

This answer suggests there might be some fiddling that can be done:

https://stackoverflow.com/a/33094994

@jffng
Copy link
Contributor Author

jffng commented Apr 21, 2020

I tested @jasmussen's idea, and it helped create some antialiasing, I think.

50:50 49.9:50
Screen Shot 2020-04-21 at 2 10 14 PM Screen Shot 2020-04-21 at 2 10 04 PM

@jasmussen
Copy link
Member

Oh wow yeah that does look slightly better! I'm sure it's a balance to get just right but the pixels appear gone.

@kjellr
Copy link
Contributor

kjellr commented Apr 21, 2020

Oh weird! I wonder why I didn't see that on my end when I tried it... this looks way better. 🎉

@allancole
Copy link
Collaborator

Yup, that looks much better now, Jeff 👍

@jffng
Copy link
Contributor Author

jffng commented Apr 22, 2020

I wonder why I didn't see that on my end when I tried it

After some more testing, it only improved when the darker color stopped earlier (i.e. 49.9%), not the other way around.

I just pushed what I thought created the best balance, and tested it across backgrounds of varying width and height. There is one use case I came across that it doesn't scale to very well: a narrow height.

Screen Shot 2020-04-22 at 9 18 31 AM

Screen Shot 2020-04-22 at 9 21 00 AM

When I try to account for this, it degrades the backgrounds at other scales. I don't think it's a dealbreaker, as most everything else looks good. Can we merge, try using it, and adjust as needed?

@kjellr
Copy link
Contributor

kjellr commented Apr 22, 2020

When I try to account for this, it degrades the backgrounds at other scales. I don't think it's a dealbreaker, as most everything else looks good.

Yeah, I think that's okay. Users will manually be selecting this gradient, so if it looks bad, they can just change this.

Can we merge, try using it, and adjust as needed?

Since we've already got this PR going, can we add just a few more so we've got all our bases covered? Some ideas:

Gradients

@jffng
Copy link
Contributor Author

jffng commented Apr 23, 2020

@kjellr I added those presets and encountered a bug in Gutenberg, filed here WordPress/gutenberg#21831.

It only happens with the horizontal hard break preset.

Copy link
Contributor

@kjellr kjellr left a comment

Choose a reason for hiding this comment

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

Awesome! These look great. I think it's fine to merge for now, even though those two gradients cause a Gutenberg bug. That'll hopefully be sorted out later, and if it isn't we can strip those two back out.

@kjellr kjellr dismissed allancole’s stale review April 23, 2020 22:48

Structure was updated. 👍

@allancole
Copy link
Collaborator

Yup, this is good to go.
Rebase against master to fix that conflict and then LGTM! 👍

@jffng jffng merged commit 06239ed into master Apr 24, 2020
@jffng jffng deleted the try/gradient-presets branch April 24, 2020 17:35
@jffng jffng mentioned this pull request Apr 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request [Theme] Varya
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants