Codeinwp / neve

A fast, lightweight, AMP ready WordPress theme built with speed and usability in mind.
https://themeisle.com/themes/neve/
GNU General Public License v2.0
262 stars 84 forks source link

Add shadow controls for primary / secondary buttons #3438

Closed JohnPixle closed 2 years ago

JohnPixle commented 2 years ago

Description:

To keep up with latest design trends and overall flexibility, I suggest we add support for Shadows in the primary and secondary buttons.

This set of controls could be added in the Customiser > Buttons panel, under the primary and secondary button panels.

Frame 8

We can get inspired for the UX from how otter does it.

Frame 9

Let me know if you need any UI help with this one!

cc @mghenciu

Alternatives:

Do nothing.

bderaguts commented 2 years ago

What about adding hover animation options too?

JohnPixle commented 2 years ago

I would not disagree about button animations if they are not too much trouble to implement. But probably just a couple of animations, nothing too fancy.

I tried to look into the customiser if we already have animation controls somewhere that we can re-use, but did not find anything though.

preda-bogdan commented 2 years ago

@JohnPixle I implemented this feature on a Draft PR however I have a few questions. I added the controls here: image

I took the liberty to not use the opacity control as it is already part of the Color control and the box-shadow CSS property doesn't have a separate property for opacity, is also part of the color. Let me know if this is ok with you.

Applying the shadow to all buttons will have some undesirable effects, for example, the buttons that are part of the search component: image

I think we should exclude those from supporting shadows else we would need to either treat the whole input as a button so that the shadow is applied consistently or allow shadow controls on form elements. There might be other cases that we would need to exclude. What is your opinion on this?

Another side issue that @selul or @abaicus could help with, is an opinion with regards to the size limit for the CSS. Since the box-shadow property is being added to the main CSS file we go over the set size limit. In this specific case since it is an inherited feature as part of the core, could we increase the size limit? If not I would need to enqueue and target all selectors only if the shadow toggle is active and add inline styles just for a button property.

What do you guys think?

cristian-ungureanu commented 2 years ago

@preda-bogdan, the selectors should be already there. We can allow shadows on the same selectors on which we apply the background color for primary/secondary buttons. If we use the same selectors, we won't need to exclude anything else, and the shadow won't apply to the search button.

In this case, I think we can increase the limit of the CSS. What do you think @selul, @abaicus?

selul commented 2 years ago

@cristian-ungureanu increasing the limit is not a good idea. We need to search for a solution that does not involve this.

As I said previously after this change should be no impact to user's page weight and we should add the style only when they use the control.

abaicus commented 2 years ago

@cristian-ungureanu @preda-bogdan @selul

The issue with this would be that adding it another way will add more inline styles than adding it straight inside the CSS stylesheet; You'd have to add selectors for all the buttons we want to add this to - we already have these selectors in the main CSS.

Could we maybe look around for other things we can extract from the CSS and add inline?

JohnPixle commented 2 years ago

Thanks for the update @preda-bogdan

I don't have any technical insights on the issues, but I only have one small comment on the labels:

Instead of width and height, I believe it would make more sense to follow some more industry-standard naming conventions. How about:

Offset X Offset Y

or

Horizontal offset Vertical offset

I am fine with any of the above.

Also, if it helps you in any way, I am perfectly fine with excluding any buttons with "supporting role", such as the buttons in search fields.

preda-bogdan commented 2 years ago

@JohnPixle I've used those labels instead of more specific ones to avoid adding more strings to the translation file. @selul @cristian-ungureanu @abaicus I can only target is-primary and is-secondary classes with an inline style that is being added if the shadow is being used, this way we would avoid issues with the shadow not working on other button elements and not increase the page weight for our existing users. I think limiting the scope of the shadow to just the primary and secondary buttons would work best for now and if required we can extend what we target.

Later Edit: Updated the PR so that the styles are inlined when the shadow is toggled, I only target primary and secondary buttons for the shadow to avoid visual issues like the one I presented for the search button.

pirate-bot commented 2 years ago

:tada: This issue has been resolved in version 3.3.0 :tada:

The release is available on GitHub release

Your semantic-release bot :package::rocket: