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(placement): hp takeover #9022

Merged
merged 3 commits into from
Jun 5, 2023
Merged

feat(placement): hp takeover #9022

merged 3 commits into from
Jun 5, 2023

Conversation

fiji-flo
Copy link
Contributor

@fiji-flo fiji-flo commented Jun 5, 2023

Summary

Support HP Takeover Placements.

@fiji-flo fiji-flo requested a review from LeoMcA June 5, 2023 15:06
Copy link
Member

@LeoMcA LeoMcA left a comment

Choose a reason for hiding this comment

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

Mostly nits, one major flag: we don't seem to use the placementData.hpFooter data anywhere - is that intentional, or the result of a late-stage abstraction? :)

@fiji-flo fiji-flo requested a review from LeoMcA June 5, 2023 16:12
Copy link
Member

@LeoMcA LeoMcA left a comment

Choose a reason for hiding this comment

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

One flag after testing on stage which it would be good to make a conscious decision on. Otherwise looks good! (The nits can be safely ignored) 🚀

<section className="place hp-main"></section>
) : (
<PlacementInner
pong={placementData.hpMain}
pong={placementData}
extraClassNames={["hp-main"]}
Copy link
Member

Choose a reason for hiding this comment

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

nit: could this be more generic and be hp-takover-banner or something?

}) {
const { backgroundColor } = placementData?.colors || {};
const css = Object.fromEntries(
[["--place-hp-main-background", backgroundColor]].filter(([_, v]) =>
Copy link
Member

Choose a reason for hiding this comment

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

nit: similarly, could this be more generic and be something like --place-hp-banner-background

@fiji-flo fiji-flo changed the title feat(placment): hp takeover feat(placement): hp takeover Jun 5, 2023
@fiji-flo fiji-flo merged commit 2ff4a7b into main Jun 5, 2023
@fiji-flo fiji-flo deleted the hp-takeover branch June 5, 2023 20:15
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.

2 participants