Skip to content

[MWPW-194266] - Sticky CTA#6002

Open
Deva309 wants to merge 10 commits into
site-redesign-foundationfrom
sticky-cta
Open

[MWPW-194266] - Sticky CTA#6002
Deva309 wants to merge 10 commits into
site-redesign-foundationfrom
sticky-cta

Conversation

@Deva309
Copy link
Copy Markdown
Contributor

@Deva309 Deva309 commented May 29, 2026 •
edited by github-actions Bot
Loading

@aem-code-sync
Copy link
Copy Markdown
Contributor

aem-code-sync Bot commented May 29, 2026 •
edited
Loading

Hello, I'm the AEM Code Sync Bot and I will run some actions to deploy your branch.
In case there are problems, just click the checkbox below to rerun the respective action.

  • Re-sync branch
Commits

@Deva309 Deva309 requested a review from a team May 29, 2026 13:33
overmyheadandbody
overmyheadandbody reviewed May 29, 2026
View reviewed changes
Comment thread libs/c2/assets/img/promo-arrow-right-black.svg Outdated
@@ -0,0 +1,3 @@
<svg xmlns="http://www.w3.org/2000/svg" width="12" height="12" viewBox="0 0 12 12" fill="none">
<path d="M11.208 5.417L7.508 1.717C7.186 1.395 6.664 1.395 6.342 1.717 6.02 2.039 6.02 2.561 6.342 2.883L8.633 5.175H1.375C.919 5.175.55 5.544.55 6 .55 6.456.919 6.825 1.375 6.825H8.633L6.342 9.117C6.02 9.44 6.02 9.961 6.342 10.283 6.503 10.444 6.714 10.525 6.925 10.525 7.136 10.525 7.347 10.444 7.508 10.283L11.208 6.583C11.53 6.261 11.53 5.739 11.208 5.417Z" fill="black"/>
Copy link
Copy Markdown
Contributor

@overmyheadandbody overmyheadandbody May 29, 2026

Choose a reason for hiding this comment

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

Since the icon is identical except the color, we could just adjust the colors via CSS, right? It doesn't look like we need two different icons.

Copy link
Copy Markdown
Contributor Author

@Deva309 Deva309 May 29, 2026

Choose a reason for hiding this comment

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

@overmyheadandbody Now, I am using the SVG directly as HTML content, which makes it easy to customize the color. However, since this icon is part of the promo CTA (with the arrow) and may be reused in other places.
Do you think maintaining these icons in icons.js is the good approach, or if it would be better to keep them in a utils.js?

Comment thread libs/mep/ace1205/floating-cta/floating-cta.js Outdated
Comment thread libs/c2/styles/styles.css Outdated
.icon-button {
align-items: center;
background-color: var(--s2a-color-transparent-white-16);
block-size: var(--s2a-spacing-xl);
Copy link
Copy Markdown
Contributor

@overmyheadandbody overmyheadandbody May 29, 2026

Choose a reason for hiding this comment

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

Better have this explicit, as spacings could change in future token package updates. Same for inline-size: var(--s2a-spacing-xl); below.

Copy link
Copy Markdown
Contributor Author

@Deva309 Deva309 May 29, 2026

Choose a reason for hiding this comment

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

Done

Comment thread libs/c2/styles/styles.css Outdated
Comment thread libs/c2/styles/styles.css Outdated
zagi25
zagi25 reviewed May 29, 2026
View reviewed changes
Comment thread libs/mep/ace1205/floating-cta/floating-cta.js
Comment thread libs/mep/ace1205/floating-cta/floating-cta.css
DKos95
DKos95 reviewed May 29, 2026
View reviewed changes
Comment thread libs/utils/utils.js Outdated
@Deva309 Deva309 changed the title Sticky cta [MWPW-194266] - Sticky CTA May 29, 2026
narcis-radu
narcis-radu approved these changes Jun 1, 2026
View reviewed changes
Comment thread libs/mep/ace1205/floating-cta/floating-cta.css
@narcis-radu narcis-radu requested a review from robert-bogos June 1, 2026 12:28
overmyheadandbody
overmyheadandbody approved these changes Jun 1, 2026
View reviewed changes
Comment thread libs/c2/styles/styles.css Outdated
.icon-button {
align-items: center;
background-color: var(--s2a-color-transparent-white-16);
block-size: var(--s2a-spacing-32);
Copy link
Copy Markdown
Contributor

@overmyheadandbody overmyheadandbody Jun 1, 2026

Choose a reason for hiding this comment

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

This was brought up before, but I see it hasn't changed. Width/height should not be based on spacing tokens, these can be explicit unit values. Same for L955

Copy link
Copy Markdown
Contributor Author

@Deva309 Deva309 Jun 1, 2026

Choose a reason for hiding this comment

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

Done

Comment thread libs/c2/styles/styles.css Outdated
text-decoration: none;
white-space: nowrap;
img {
block-size: var(--s2a-spacing-32) ;
Copy link
Copy Markdown
Contributor

@overmyheadandbody overmyheadandbody Jun 1, 2026

Choose a reason for hiding this comment

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

Suggested change
block-size: var(--s2a-spacing-32) ;
block-size: var(--s2a-spacing-32);

And the same comment around using spacing tokens for width/height, it should be avoided. Since I see this repeating, you can just scope a variable with the desired size to the block itself, like

.icon-button {
  --icon-size: 32px;
  ...
}

Copy link
Copy Markdown
Contributor Author

@Deva309 Deva309 Jun 1, 2026

Choose a reason for hiding this comment

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

Done

Comment thread libs/mep/ace1205/floating-cta/floating-cta.js Outdated
const ctaHref = linkEl?.getAttribute('href') || '#';

const arrow = createTag('span', { class: 'icon-button', 'aria-hidden': 'true' });
arrow.innerHTML = icons.arrowRightWhite;
Copy link
Copy Markdown
Contributor

@overmyheadandbody overmyheadandbody Jun 1, 2026

Choose a reason for hiding this comment

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

This could cause a false positive from Kodiak, you could likely add this through createTag

Copy link
Copy Markdown
Contributor Author

@Deva309 Deva309 Jun 1, 2026

Choose a reason for hiding this comment

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

Done

robert-bogos
robert-bogos reviewed Jun 1, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@robert-bogos robert-bogos left a comment

Choose a reason for hiding this comment

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

some a11y issues

Comment thread libs/mep/ace1205/floating-cta/floating-cta.css
Comment thread libs/mep/ace1205/floating-cta/floating-cta.js Outdated
@Deva309 Deva309 requested a review from robert-bogos June 1, 2026 14:13
robert-bogos
robert-bogos approved these changes Jun 1, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@robert-bogos robert-bogos left a comment

Choose a reason for hiding this comment

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

Thank you for taking care of the feedback ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@narcis-radu narcis-radu narcis-radu approved these changes

@overmyheadandbody overmyheadandbody overmyheadandbody approved these changes

@zagi25 zagi25 zagi25 approved these changes

@robert-bogos robert-bogos robert-bogos approved these changes

@DKos95 DKos95 Awaiting requested review from DKos95

Assignees

@SilviuLCF SilviuLCF

Labels

Site Redesign

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@Deva309 @narcis-radu @overmyheadandbody @zagi25 @robert-bogos @DKos95 @SilviuLCF