-
Notifications
You must be signed in to change notification settings - Fork 163
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-158053: CCD Slice Card [Merch card] #2878
base: stage
Are you sure you want to change the base?
Conversation
- move from merch-card.js code each layout code related to a given variant under variants/<variant> class - move from global.css.js css related to a given variant under variants/<variant>.css.js - move from merch-card.css.js wc css related to a given variant under variants/<variant>.variantStyle - uses variants/variants as an index to get related class from related variant, and wc style too, - uses variants/VariantLayout, superclass of all variants, to insert css if card is used, and hold common code
to something independent of the layout (not initialized when used at first in collections)
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.
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.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## stage #2878 +/- ##
==========================================
- Coverage 96.25% 95.91% -0.35%
==========================================
Files 238 172 -66
Lines 54310 46390 -7920
==========================================
- Hits 52276 44493 -7783
+ Misses 2034 1897 -137 ☔ 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.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
This pull request is not passing all required checks. Please see this discussion for information on how to get all checks passing. Inconsistent checks can be manually retried. If a test absolutely can not pass for a good reason, please add a comment with an explanation to the PR. |
Hi @Axelcureno, just a suggestion, but if we wait for #2894 we can add a test page showcasing the CCD cards you've implemented |
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 might not need the images for unit tests, as discussed before with Milo core team.
rest looks good to me.
p.s. for PSI check - we can create a test page with odin resources, once #2894 is ready
grid-template-columns: var(--consonant-merch-card-ccd-slice-width); | ||
} | ||
|
||
merch-card[variant="ccd-slice"] [slot='body-s'] { |
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 wonder if we shouldn't try to move slot styling if possible into variantStyles? cc @yesil
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 agree, anything that can stay in the shadowRoot should go in variantStyles.
constructor(card) { | ||
super(card); | ||
} |
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.
this can go away :)
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 are still under discussion (well not tracked atm) with milo-core team wrt to images in repository, waiting for this, let's not add new ones. Do you think you can handle it with the one we have? (even if they are not square)?
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 will create a discussion, thanks @npeltier !
align-self: center; | ||
} | ||
|
||
@media screen and ${TABLET_UP} { |
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.
These styles are needed to display the variant in the responsive grid. However afaik, CCD is not going to use our responsive grid but put a merch-card in their own container.
In order to keep our codebase as simple as possible, I suggest to remove these styles. (lines 45-64 and 12-18)
} | ||
|
||
@media screen and (min-width: 1440px) { | ||
#sidenav { |
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.
let's remove sidenav, and keep this as simple as possible.
mockLana(); | ||
await mockFetch(withWcs); | ||
await mas(); | ||
appendMiloStyles(); |
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 should remove appendMiloStyles, since mas is in milo now. Someone needs to remove it all together.
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.
Removed.
<body> | ||
<script type="module" src="./merch-card.ccd-slice.test.html.js"></script> | ||
<main> | ||
<div class="one-merch-card ccd-slice"> |
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 think we don't need this div neither since we do not add responsive grid styles for CCD cards.
</merch-card | ||
> | ||
</div> | ||
<div class="one-merch-card ccd-slice-wide"> |
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 think we don't need this div neither since we do not add responsive grid styles for CCD cards.
padding-top: 80px; | ||
} | ||
|
||
main>div { |
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.
probably not needed neither.
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.
@npeltier until the discussion actually happens, I would like to proceed with merging the PR.
Test images have no impact on Milo's performance or stability and dev preferences should not be a reason to block PRs.
@Axelcureno please check MAS Unit Tests, they are green but reported as failure. |
Introduces Creative Cloud Desktop 'Slice' Card to the list of web components as part of M@S v2 initiative.
LTR:
RTL:
Notes:
npm run test
from web-components folder and select debug, then select the optiontest/merch-card.ccd-slice.test.html
to see the 2 options (standard size and wide)Resolves: MWPW-158053
Test URLs: