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

feat(cxl-ui): cxl-section - implement hero section design facelift #365

Merged
merged 4 commits into from
Dec 7, 2023

Conversation

freudFlintstone
Copy link

No description provided.

@freudFlintstone freudFlintstone self-assigned this Nov 23, 2023
@freudFlintstone freudFlintstone changed the title Raphael/refactor/facelift feat(cxl-ui): implement design facelift changes Nov 23, 2023
@freudFlintstone freudFlintstone marked this pull request as draft November 23, 2023 21:22
@freudFlintstone freudFlintstone changed the title feat(cxl-ui): implement design facelift changes feat(cxl-ui): cxl-section - implement header design facelift Nov 23, 2023
@freudFlintstone
Copy link
Author

Copy link

github-actions bot commented Nov 26, 2023

size-limit report 📦

Path Size
packages/cxl-ui/pkg/dist-web/cxl-ui.js 67.49 KB (+0.77% 🔺)
packages/cxl-ui/pkg/dist-web/cxl-ui-jwplayer.js 11.87 KB (0%)
packages/cxl-ui/pkg/dist-web/cxl-ui-playbooks.js 28.05 KB (-0.31% 🔽)
packages/cxl-ui/pkg/dist-web/vendor.js 135.58 KB (0%)
packages/cxl-ui/pkg/dist-web/cxl-ui-jwplayer.js, packages/cxl-ui/pkg/dist-web/cxl-ui-playbooks.js, packages/cxl-ui/pkg/dist-web/cxl-ui.js, packages/cxl-ui/pkg/dist-web/manifest.js, packages/cxl-ui/pkg/dist-web/unresolved.js, packages/cxl-ui/pkg/dist-web/vendor.js 244.15 KB (+0.18% 🔺)

@freudFlintstone
Copy link
Author

@lkraav
Copy link

lkraav commented Nov 29, 2023

By "Header", are we really meaning "Hero" here? "Header" is the part where navigation lives.


&::before {
right: 47%;
height: 60%;

Choose a reason for hiding this comment

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

Rather random percentages, but I hope they were thoroughly considered.

Also, was max-height ever discussed?

Copy link
Author

@freudFlintstone freudFlintstone Nov 29, 2023

Choose a reason for hiding this comment

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

Yes, @heshfekry and I tested them during a call, to try matching them to the design. However, those values have since indeed changed, to improve visual balance and make the hero image more prominent.

You're are right, max-height would have worked fine in a more conventional scenario, but relying on dynamic values made it easier to handle responsiveness without adding several break points, and aligning the bottom of the element with the bottom of the image.

@freudFlintstone freudFlintstone changed the title feat(cxl-ui): cxl-section - implement header design facelift feat(cxl-ui): cxl-section - implement hero section design facelift Nov 29, 2023
@freudFlintstone freudFlintstone force-pushed the raphael/refactor/facelift branch 3 times, most recently from 936b9e9 to d38bed6 Compare November 29, 2023 21:17
@freudFlintstone
Copy link
Author

In the latest commit , I set up a way to avoid having to go through each page in the block editor to apply the update.

  1. Added global scope CSS to hide the background image already set in the post
  2. By adding the following inline style to each id="view-hero" cxl-section (similar to how @pawelkmpt added the wide-background-<color> in PHP), hero section will get the updated design without any further work. It will also make any subsequent changes much easier.
style="--hero-image: url([wpv-post-featured-image size="large" output="url"])"

This is not ideal, by any means, but it's the least changes required, given we already have a lot going on in the code base and in wordpress. Were this being done from scratch, cxl-section[theme="cxl-hero"] should have some basic scaffolding template and we would pass the URL as a property.

Copy link

@pawelkmpt pawelkmpt left a comment

Choose a reason for hiding this comment

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

packages/cxl-lumo-styles/scss/_mixins.scss Outdated Show resolved Hide resolved
@freudFlintstone freudFlintstone force-pushed the raphael/refactor/facelift branch 2 times, most recently from 4cd9514 to 0ea19e3 Compare December 4, 2023 20:56
@pawelkmpt pawelkmpt merged commit 949167d into master Dec 7, 2023
5 checks passed
@pawelkmpt pawelkmpt deleted the raphael/refactor/facelift branch December 7, 2023 06:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants