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

Light overlay update #33

Merged
merged 1 commit into from
Apr 17, 2024
Merged

Light overlay update #33

merged 1 commit into from
Apr 17, 2024

Conversation

patrickbrown-io
Copy link
Contributor

Modifies the Light Overlay on section background images to be more white than gray so particularly light background images are no longer darkened by the overlay due

Resolves #32

@patrickbrown-io patrickbrown-io added the change Modification to code without a functional change label Apr 17, 2024
@timurtripp timurtripp self-requested a review April 17, 2024 17:01
Copy link
Member

@timurtripp timurtripp left a comment

Choose a reason for hiding this comment

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

It worked but required me to resave the section to apply the change

@timurtripp
Copy link
Member

timurtripp commented Apr 17, 2024

@jnicholCU Before I merge this, is there any way around the resave issue? Any reason this isn't done using CSS classes or variables?

@jnicholCU
Copy link
Contributor

@jnicholCU Before I merge this, is there any way around the resave issue? Any reason this isn't done using CSS classes or variables?

@timurtripp originally there wasn't a better way of doing it for that particular option. I had worked with another custom layout builder dev on this problem because we ran into the same issue with background images with color overlays.
That being said I'm looking at new documentation and there might be a more clean way.

For now I think this is fine to merge and we can open up a ticket for a refactor that I can link the documentation to. The migration stuff that Michael and Jeremy have worked on are using the current method so if we change it that will need to be redone. Probably be worth talking about on Friday or Tuesday.

@timurtripp
Copy link
Member

Alright. Next time you should use CSS variables, I believe it could be done that way here even if inline styles are required due to the background image. CSS variables are allowed in inline styles and it would've prevented this problem from occurring.

@timurtripp timurtripp merged commit 7aab50f into main Apr 17, 2024
2 checks passed
@timurtripp timurtripp deleted the issue/32 branch April 17, 2024 18:18
github-actions bot pushed a commit that referenced this pull request Apr 17, 2024
@timurtripp
Copy link
Member

I'd go so far as to say almost anything color-related should be defined a variable and never as a hard-coded value, we reuse the same colors a lot across the site.

@timurtripp
Copy link
Member

You see done like this all over the web now. Not using CSS variables will date you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
change Modification to code without a functional change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Light background overlay makes light images darker
3 participants