-
Notifications
You must be signed in to change notification settings - Fork 173
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
Mwpw 164093: Adobe Home 'Pricing Widget' merch card #3384
base: MWPW-163479
Are you sure you want to change the base?
Conversation
Hello, I'm the AEM Code Sync Bot and I will run some actions to deploy your branch and validate page speed.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remaining comments which cannot be posted as a review comment to avoid GitHub Rate Limit
eslint
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## MWPW-163479 #3384 +/- ##
=============================================
Coverage 96.47% 96.48%
=============================================
Files 254 256 +2
Lines 59090 59239 +149
=============================================
+ Hits 57006 57154 +148
- Misses 2084 2085 +1 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remaining comments which cannot be posted as a review comment to avoid GitHub Rate Limit
eslint
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
libs/features/mas/src/global.css.js
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tend to put test code for variants in the same .test.html file since there is not much code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do this
<sp-button size="s" variant="accent">Buy</sp-button> | ||
</div> | ||
</merch-card> | ||
<merch-card class="" tabindex="0" variant="pricing-widget"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is empty class=""
needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing..
<body> | ||
<script type="module" src="./merch-card.pricing-widget.test.html.js"></script> | ||
<main> | ||
<sp-theme color="light" scale="medium" dir="ltr"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if Adobe Home goes with Spectrum CSS, we should probably update this file for Spectrum CSS instead of SWC, to be checked with AH.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we assume in the meantime that we are going with Spectrum CSS?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you try to confirm with AH team?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
file name is too generic, can we prefix with ah
? Also IMOwidget
is not needed in the name.
suggestion: ah-pricing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree with 'ah' prefix idea, even if it will be used on other surface its good to 'mark' original card owner.
for POC imo ah-pricing-widget
is ok, later AH should come up with the final name for this card. In the call with their dev he mentioned that they might want to name it after the page its used on, similar to how we have a catalog or plans card, lets wait for their feedback
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
flex-direction: column; | ||
font-size: 12px; | ||
line-height: 18px; | ||
color: var(--spectrum-gray-800); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we struggled with CCD colors, can you set colors using RGB values via the CSS variables?
@@ -0,0 +1,36 @@ | |||
export const CSS = ` | |||
:root { | |||
--merch-card-pricing-widget-width: 132px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
default values for pricing variant specific variables, can be defined at https://github.com/adobecom/milo/pull/3384/files#diff-e60be84bbf9d85ac9cd9234dec52df2fedf7fa167bd8eb18a9f67886c84086b9R40
:root
is the top scope, and we don't need to define these variables there.
color: var(--spectrum-gray-800); | ||
} | ||
|
||
merch-card[variant="pricing-widget"] [slot="body-xxs"] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are we duplicating all the styles here?
if the only thing that changes is the color, we can redefine the color variables here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this particular card slot, there is a requirement to trim the text and display ellipsis when the content length is bigger than the space available 54px
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Axelcureno could you please add an Adobe Home gallery html and nala test for it?
just one card gallery and some simple minimal test would be good
otherwise looks good to me, thank you
@Axelcureno @yesil @3ch023 should we create an AH branch once this is accepted so they can start playing around? |
libs/features/mas/src/hydrate.js
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
Sure, I have created new gallery page for Adobe Home (and addressed all other comments): https://mwpw-164093--milo--adobecom.hlx.page/libs/features/mas/docs/adobe-home.html |
@@ -82,6 +82,12 @@ export function processSubtitle(fields, merchCard, subtitleConfig) { | |||
} | |||
} | |||
|
|||
export function processBackgroundColor(fields, merchCard, allowedColors) { | |||
if (allowedColors?.includes(fields.backgroundColor)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not fail fast?
max-height: 54px; | ||
overflow: hidden; | ||
text-overflow: ellipsis; | ||
display: -webkit-box; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about Firefox?
This PR introduces Adobe Home 'Pricing Widget' merch card.
Resolves: MWPW-164093
M@S Studio Test URL: https://mwpw-164093--mas--adobecom.hlx.page/studio.html?milolibs=MWPW-164093--milo--adobecom#path=adobe-home&query=pricing
Test URLs: