Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow transparent backgrounds for banners #1796

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

allylane
Copy link
Contributor

@allylane allylane commented Jun 14, 2022

PR Summary:

Removing the 0.1 opacity default background, and "magic" colours when overlays are applied so they stay true to the colour palletes.

Why are these changes introduced?

More creativity with banners

Testing steps/scenarios

  • Open the editor
  • Click on the image banner
  • Upload a transparent png for an image

Alistair Lane added 2 commits June 14, 2022 15:03
So we can use transparent images.
@allylane
Copy link
Contributor Author

Transparentbanners

Copy link
Contributor

@Oliviammarcello Oliviammarcello left a comment

Choose a reason for hiding this comment

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

Looks good and works as expected!

  • We should now remove the help text under the colour scheme picker since the selector will always work regardless of the container. Screenshot

Copy link
Contributor

@ludoboludo ludoboludo left a comment

Choose a reason for hiding this comment

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

I think we might need to change the default values of that section with a colour scheme using a white text, screenshot. But that would need to be done in the private dawn repo I believe as it would be an update on the .json file.

.banner--desktop-transparent .button--secondary {
--color-button: 255, 255, 255;
--color-button-text: 255, 255, 255;
--alpha-button-background: 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should be removing the alpha value here. If the button is using the outline style it should keep a transparent background 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

On mobile it looks good but not on desktop: recording.

Comment on lines -321 to -323
--color-foreground: 255, 255, 255;
--color-button: 255, 255, 255;
--color-button-text: 0, 0, 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Those changes are on declarations within a media query @media screen and (min-width: 750px) { so they're only impacting the wider screens.
If you're on mobile the colour scheme is never applied and the default is white.

So it can make for a confusing experience. As it's working on desktop and applying the colour scheme accordingly when a container is and isn't showing but not on mobile.

Also the info next to the colour scheme setting needs to be updated:

@andrewetchen
Copy link
Contributor

andrewetchen commented Jul 27, 2022

Hey team,

The probot-CLA was recently deprecated and replaced with the shopify-cla action.

To successfully use the new shopify-cla status check, this PR will be closed and reopened.

Thanks and sorry for the minor inconvenience. Message me if you have any questions 😄

Please refer to the following for more information:

@andrewetchen andrewetchen reopened this Jul 27, 2022
@LucasLacerdaUX LucasLacerdaUX removed their request for review May 31, 2023 13:59
@martinamarien martinamarien removed their request for review March 6, 2024 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants