Codeinwp / neve-fse

The theme you already love, built for Full-Site Editing
15 stars 6 forks source link

Changes the color styles of the sections on the homepage + Adds new Styles #59

Closed mghenciu closed 3 months ago

mghenciu commented 1 year ago

Summary

Will affect visual aspect of the product

YES

Screenshots

Before

Screenshot 2023-07-18 at 14 44 41

After

Screenshot 2023-07-18 at 14 42 29

Test instructions

Closes #71 .

This should fix the issue where in the WP repo, previewing colors will make the Nav Bar look different compared to the Icons section (examples below where it's visible that things can be improved):

Images Screenshot 2023-07-18 at 14 46 38 ___ Screenshot 2023-07-18 at 14 47 18

Closes #71


@JohnPixle , when you have some time, please let me know if this is ok - and I'll pass it to testing. 🙇 After this I'll create also a new PR with more color styles and small updates on the current ones.

github-actions[bot] commented 1 year ago

Plugin build for 2a567b367eefb3d26a2ef8f987f792c8f9256554 is ready :bellhop_bell:!

JohnPixle commented 1 year ago

Thanks @mghenciu this looks good, I just tested it on a taste instance.

I wonder if/how this change affects the existing installations 🤔 . I initially installed the PR in an existing NeveFSE installation/setup but the changes did not reflect in the homepage template. Magic Link.

The patterns were updated fine though, I see they are getting inserted with the right colors. But the homepage template remains the same.

Thanks!

mghenciu commented 1 year ago

@JohnPixle , I finished experimenting here. When you have some time, please take a look. Here's a quick vide with styles:

https://github.com/Codeinwp/neve-fse/assets/52494172/670c2617-37d2-4625-b8c3-34c27fbd290a

Duotone example https://github.com/Codeinwp/neve-fse/assets/52494172/29204fec-2863-41ce-ae5a-65ae0610b902

Key changes


Other comments

Other things that I noticed while doing testing, and I think we need to address those (I can make issue if you agree):


For the Dev team before merging

cc @HardeepAsrani

JohnPixle commented 1 year ago

Thanks for the effort here @mghenciu great work on the design and styles. I noticed the spacing issue as well. It's a deal-breaker, we need some help here.

Screenshot 2023-08-25 at 4 50 01 PM

irinelenache commented 1 year ago

@mghenciu Tested and found only one thing, don't know if it's an actual issue:

If it's safe to leave it like this, let me know and i'll move this to Ready to merge 🚀

JohnPixle commented 1 year ago

@irinelenache Thanks for looking into this. Just fyi, the text color behaviour is expected, but thanks for pointing it out. It's built-into the core Cover block and it adjusts the color automatically based on contrast.

For this release, we will not include this PR however, we need a bot more time to experiment with duotones as a concept.

In my instance, I also noticed that despite the fact that I am using theme colors in a duotone filter, the color is not dynamically preserved when switching theme styles. Does it happen to you as well?

irinelenache commented 1 year ago

@JohnPixle Thanks for the clarification 🙏

In my instance, I also noticed that despite the fact that I am using theme colors in a duotone filter, the color is not dynamically preserved when switching theme styles. Does it happen to you as well?

^ I checked now and it happens to me also

JohnPixle commented 1 year ago

@irinelenache ok, good to know. We might need to register our own duotone presets, we will do in the next update. Thanks for confirming 🙏🏻

mghenciu commented 1 year ago

In my instance, I also noticed that despite the fact that I am using theme colors in a duotone filter, the color is not dynamically preserved when switching theme styles. Does it happen to you as well?

When you have some time, @JohnPixle , could you please share a bit more details about this.

I've tried changing the duotone, but I think you are referring to something else:

https://github.com/Codeinwp/neve-fse/assets/52494172/33e9def6-c57f-4b02-afae-f6d0756da4dd

JohnPixle commented 1 year ago

@mghenciu yeah, this does not appear to be working on my end, or Irinel's 🤔 Good to see it works for you at least. I assume we would need to register our own theme-duotones and gradients in order to achieve that.

irinelenache commented 1 year ago

@mghenciu @JohnPixle For me the issue was something like this:

JohnPixle commented 1 year ago

Yeah Irinel, your expectations were right. That was my issue as well, that theme colors in duotones and gradients are not dynamically adapting to the colors of each theme style.

JohnPixle commented 1 year ago

We got it sorted with Mihai eventually, it works at his end because he is using the PR which has a registered theme duotone. In this case it works. We can still wait for this in the next release, as we decided not to apply duotone by default to the homepage hero, but instead experiment with other patterns first.

Thanks!