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.

Sign up for GitHub

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

Jump to bottom

MWPW-136866: merch-cards block #1729

Merged
merged 3 commits into from
Jan 15, 2024
Merged

MWPW-136866: merch-cards block #1729

merged 3 commits into from
Jan 15, 2024

Conversation

yesil
Copy link
Contributor

@yesil yesil commented Jan 11, 2024

@yesil
MWPW-136866: merch-cards block (adobecom#1676)
7290a28 
* MWPW-136866: merch-cards block

* update code owners

* fix failing tests

* rename main to init
@yesil yesil requested a review from a team as a code owner January 11, 2024 14:23
@yesil yesil changed the title MWPW-136866: merch-cards block (#1676) MWPW-136866: merch-cards block Jan 11, 2024
@yesil yesil added new-feature New block or other feature needs-verification PR requires E2E testing by a reviewer commerce labels Jan 11, 2024
@skumar09 skumar09 added the run-nala Run Nala Test Automation against PR label Jan 12, 2024
honstar
honstar reviewed Jan 15, 2024
View reviewed changes
libs/blocks/merch-cards/merch-cards.js

// allows improve TBT by returning control to the main thread.
// eslint-disable-next-line no-promise-executor-return
const makePause = async () => new Promise((resolve) => setTimeout(resolve, 0));
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you share some data around this TBT optimization? It'd be interesting to understand the impact of interrupting processing in the initMerchCards function further down.

Copy link
Contributor

Choose a reason for hiding this comment

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

i think it would deserve some central discussion (and feature!) as it as been used by @narcis-radu and @mokimo iirc on feds

Copy link
Contributor

Choose a reason for hiding this comment

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

(not saying we need feature/discussion before end of that PR)

Copy link
Contributor

Choose a reason for hiding this comment

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

i think it would deserve some central discussion (and feature!) as it as been used by @narcis-radu and @mokimo iirc on feds

Right, a TBT optimization discussion sounds like a good idea 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@honstar we have around 93 cards(fragment, block) in the catalog. We don't need to initialise all of them at the same time to render. It is increasing TBT considerably.
Here by interrupting the processes and giving the UI thread chance to do something else, we keep the TBT under control.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, this makes a lot of sense. Eventually, we might want to consider lazy-loading larger sets of collections but that is for another day ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the thing is, we don't know which card is the first card, and the card metadata must be initialised so that we know whether it must be displayed or not.
So we should still eagerly initialise them as early as possible, but with mini fake pauses :)

honstar
honstar reviewed Jan 15, 2024
View reviewed changes
libs/blocks/merch-cards/merch-cards.js
}
));

const appContainer = document.querySelector('.merch.app');
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this .merch.app container? Is this driven by authoring?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, basically if a merch block finds a .merch.app element in the DOM, it will go stick to it instead of being rendered in-place.

sidenav in cc do the same so that we can achieve the desired layout.

For the catalog page, we have a dumb block serving as a merch.app container.

npeltier
npeltier reviewed Jan 15, 2024
View reviewed changes
libs/blocks/merch-cards/merch-cards.css
}

merch-cards {
min-height: 200px;
Copy link
Contributor

Choose a reason for hiding this comment

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

where is that 200px come from?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@npeltier it is an arbitrary value to allocate some space for the cards.
in a second PR,, I'll adjust spacing after feedback from different stakeholders and more visual tests.

@afmicka afmicka added the verified PR has been E2E tested by a reviewer label Jan 15, 2024
@yesil yesil merged commit 826bd49 into adobecom:main Jan 15, 2024
10 of 11 checks passed
narcis-radu pushed a commit that referenced this pull request Jan 17, 2024
@robert-bogos @rgclayton
@Blainegunn @elan-tbx @ryanmparrish @vgoodric @vivgoodrich @3ch023 @Ruchika4 @JackySun9 @overmyheadandbody @mokimo @npeltier @meganthecoder @Sartxi @Axelcureno @seanchoi-dev @Brandon32 @aishwaryamathuria @yesil @suhjainadobe @chrischrischris @skumar09 @nateekar
Sync gnav branch from main (#1744)
1097e1f 
* MWPW-137938 - Inline video image fallback (#1542)

* Summarize changes in 50 characters or less

More detailed explanatory text. Wrap at 72 characters.

Resolves: MWPW-xxxx
See also: MWPW-xxxx, MWPW-xxxx

* Adding unit test. Addressing PR feedback.

---------

Co-authored-by: Ryan Clayton <[email protected]>
Co-authored-by: Blaine Gunn <[email protected]>

* MWPW-137523 - Allow dark quiet marquees (#1654)

* apply style updates

* do not add dark to quiet marquee by default

* additional :is usage

* MWPW-140321 - how-to block - Heading area without Image has gap (#1670)

Added style for .has-class to set logical grid display

* MWPW-140273 Highlight replace fragment (#1672)

* MWPW-140273 highlight replace fragment

* getFileName function

* permissions test

* remove test comment

* make manifestId consistent

* fix unit tests

* add to unit test

* streamline addHighlightData

---------

Co-authored-by: vivgoodrich <[email protected]>

* MWPW-140470: allow OST to overwrite landscape (#1668)

allow OST to overwrite landscape

* MWPW-131662: consistent global navigation padding on mobile (#1679)

* fixed global navigation padding on mobile

* hotfix

* hotfix

* hotfix

* hotfix

* MWPW-131195: gnav cloud links section (#1586)

* ui: gnav cloud links

* hotfix

* hotfix

* hotfix

* hotfix

* hotfix

* hotfix

* hotfixes

* hotfixes

* hotfix

* hotfixes

* optimizing

* hotfix

* optimizing

* hotfix

* updated test mocks for cross cloud menu

* hotfix

* hotfix

* hotfix

* optimizing

* optimizing

* config - target file for testing

* Revert "MWPW-137523 - Allow dark quiet marquees" (#1693)

Revert "MWPW-137523 - Allow dark quiet marquees (#1654)"

This reverts commit 921efe5.

* MWPW-140052-Word break css for korean (#1675)

* Update styles.css

* update css to support language based korean sites too

* [MWPW-140706] Quiz options should use Adobe Clean and [MWPW-140499] fix multiple merch card width issue on result page (#1694)

* MWPW-140499: fix multiple merch card width issue on result page (#1660)

fix multiple merch card width issue on result page

Co-authored-by: xiasun <[email protected]>

* MWPW-140706: Quiz options should use Adobe Clean (#1682)

---------

Co-authored-by: xiasun <[email protected]>
Co-authored-by: Erich Champion <[email protected]>

* [MWPW-140788] Ensure correct Gnav Promo element order (#1691)

* [MWPW-140788] Ensure correct Gnav Promo element order

* [MWPW-140788] Unit tests for Gnav Promo elem order

* Fix broken gnav test (#1699)

* Fix broken gnav test

* Cleanup

* MWPW-140877 - Quiz Marquee style updates (#1701)

* fixed quiz-marquee styles - aside.notification to inline on mobile, cards alignment on center

* no margin on nested aside body-m

* width update

* added same card width style to quiz-results

* [#1641] add parent option to createTag (#1698)

- add options parameter to createTag signature, with only one parent option for now, as discussed in https://github.com/orgs/adobecom/discussions/1641,
- add unit test for create Tag

* MWPW-140345-Fix issues in media QR block variant (#1658)

* MWPW-140345

* MWPW-140345

* MWPW-140345

* Update media.js

* Update media.css

* Update unit test

* Update qr block css

* Update media.css

* Update media.test.js

* Update media.test.js

* document OST overrides (#1692)

* document OST overrides
* fix typo

* Create merch block README.md (#1707)

* Create merch block README.md

* fix code styling

* MWPW-140623 - Auto-merge main to stage (#1671)

add workflow to auto-merge main into stage

* MWPW-140780 - Remove :has() selector from Aside (#1687)

remove :has selector altogether

* MWPW-140660 Fix Marketo Destination Url (#1702)

* MWPW-140862: Updated twitter logo (#1700)

[MWPW-140862] Updated twitter logo

* MWPW-140538 - Icon Block inline margin (#1688)

Unset margin auto on section up nested inline iconblocks

* Update magma codeowners (#1709)

* MWPW-137523 - Allow dark quiet marquees (redux) (#1696)

* achieve goals with lowest impact possible

* move dark exclusions to a variable

* revert min-height changes

* quiet:not(.dark)

* MWPW-140191: Fixes Merch Card custom <hr> color, inline heading variant (#1710)

MWPW-140191: Fixes Merch Card custom <hr> color

* faas-env default qa (#1716)

* faas-env default qa

* unit test

* remove colsole log

* console.error -> window.lana.log

* altering faasCurrentJS instead faasHostUrl that being exported.

* Added missed part.

* MWPW-140964 Marquee static links should not be blue on hover (#1705)

Co-authored-by: vivgoodrich <[email protected]>

* MWPW-140738 - Add foreground gap on large marquees (#1718)

remove gap: 0 on large marquees

* MWPW-139379 Improve Tab Scrolling for Mobile (#1624)

* MWPW-139379 Improve Mobile Tab Scrolling

* MWPW-140403 | Aside promo block button and block size (#1680)

For aside promo block following minor bugs are fixed in this PR

Setting default button height for all promo to be 40px (button-l).
Setting popup promo default width in desktop to be 85 % and max upto 1600px
Resolves: MWPW-140403

Test URLs:

Before: https://main--milo--adobecom.hlx.page/docs/library/kitchen-sink/aside-promo?martech=off
After: https://mwpw-140403--milo--aishwaryamathuria.hlx.page/docs/library/kitchen-sink/aside-promo?martech=off

* MWPW-140836: fix Commerce for several geo's (#1708)

* fix WCS  commerce for umbrella regions

* MWPW-139915: Delivery of Spectrum Web Components (#1663) (#1721)

* MWPW-139915: Delivery of Spectrum Web Components (#1663)

* MWPW-139915: Spectrum Web Components delivery

for commerce

* add README

* muted eslint errors for src/dist folders

* Update merch.js

* Update merch.js

* exclude swc from codecov

* PR feedbacks incorporated.

* MWPW-140267 Update MEP PZN tags (#1724)

* MWPW-140267 Update PZN tags

* update mobile definition

* add windows and mac

* base phone vs tablet on screen size

* switch to OR

* alert to debug on iphone

* tyring to test version

* remove comment now that I see update

* trying to differentiate between safari and chrome

* reverting changes for Chrome Safari differentiating

* adding feedback to debug safari on ipad

* fix false positive on logged out

* remove debug line

* changing msedge to edge

* update device type tags to all be singular

* update reference for mobile

---------

Co-authored-by: vivgoodrich <[email protected]>

* MWPW-140467 Integrate round-corners for brick block (#1664)

* Update blocks.js

* rounded-corner-brick block

* rounded-corner-brick block

* rounded-corner-brick block

* minor change

* review suggestion

---------

Co-authored-by: Suhani Jain <[email protected]>
Co-authored-by: Suhani Jain <[email protected]>

* MWPW-140953: Updated twitter logo (#1732)

ui: updated twitter logo

* MWPW-140836: Switch Milo Commerce to own country/language mapping (#1728)

* bring mapping for all wcs locales

* Revert "MWPW-139379 Improve Tab Scrolling for Mobile" (#1733)

* [MWPW-140624] Footer layout with few columns (#1730)

* MWPW-140916, MWPW-141124: Adding result to quiz analytics; update merch card width (#1717)

* MWPW-140499: fix multiple merch card width issue on result page (#1660)

fix multiple merch card width issue on result page

Co-authored-by: xiasun <[email protected]>

* MWPW-140706: Quiz options should use Adobe Clean (#1682)

* MWPW-140916: Adding result to analytics (#1715)

* MWPW-140916: Adding result to analytics

* MWPW-140916: Fixing lint errors

* MWPW-141124: match widths used by standard merch cards (#1725)

Co-authored-by: xiasun <[email protected]>

* Incorporating comments from Rares

* Updating handleResultFlow test

---------

Co-authored-by: Jacky Sun <[email protected]>
Co-authored-by: xiasun <[email protected]>
Co-authored-by: xiasun <[email protected]>

* MWPW-140268 Icon duplicates if nested in multiple sections (#1731)

* MWPW-140268 Icon duplicates if nested in multiple sections

* Update libs/features/icons/icons.js

Co-authored-by: Okan Sahin <[email protected]>

* Update unit test to check for duplicates

---------

Co-authored-by: vivgoodrich <[email protected]>
Co-authored-by: Okan Sahin <[email protected]>

* MWPW-141139 Parameter PZN tags don't match if user uses uppercase characters (#1726)

* MWPW-141139 Parameter PZN tags don't match if user uses upercase characters

* conditional chaining and param name not case sensitive

* Update libs/features/personalization/personalization.js

Co-authored-by: Chris Peyer <[email protected]>

---------

Co-authored-by: vivgoodrich <[email protected]>
Co-authored-by: Chris Peyer <[email protected]>

* MWPW-136866: merch-cards block (#1729)

* MWPW-136866: merch-cards block (#1676)

* MWPW-136866: merch-cards block

* update code owners

* fix failing tests

* rename main to init

* removed TODO comment.

* MWPW-141140 MEP throws error if page filter uses single asterisks (#1740)

Co-authored-by: vivgoodrich <[email protected]>

* [MILOLIBS RUN : MWPW-141223] Provide support to run nala tests on a given milo libs (#1742)

nala-milo-libs

Co-authored-by: nateekar <[email protected]>

---------

Co-authored-by: Ryan Clayton <[email protected]>
Co-authored-by: Ryan Clayton <[email protected]>
Co-authored-by: Blaine Gunn <[email protected]>
Co-authored-by: Elan Bartholomew <[email protected]>
Co-authored-by: Ryan Parrish <[email protected]>
Co-authored-by: Vivian A Goodrich <[email protected]>
Co-authored-by: vivgoodrich <[email protected]>
Co-authored-by: Mariia Lukianets <[email protected]>
Co-authored-by: Ruchika Sinha <[email protected]>
Co-authored-by: Jacky Sun <[email protected]>
Co-authored-by: xiasun <[email protected]>
Co-authored-by: Erich Champion <[email protected]>
Co-authored-by: Rares Munteanu <[email protected]>
Co-authored-by: Okan Sahin <[email protected]>
Co-authored-by: Nicolas Peltier <[email protected]>
Co-authored-by: Megan Thomas <[email protected]>
Co-authored-by: Sean Archibeque <[email protected]>
Co-authored-by: Axel Cureno Basurto <[email protected]>
Co-authored-by: Sean Choi <[email protected]>
Co-authored-by: Brandon Marshall <[email protected]>
Co-authored-by: Aishwarya Mathuria <[email protected]>
Co-authored-by: ilyas Stéphane Türkben <[email protected]>
Co-authored-by: Suhani Jain <[email protected]>
Co-authored-by: Suhani Jain <[email protected]>
Co-authored-by: Suhani Jain <[email protected]>
Co-authored-by: xiasun <[email protected]>
Co-authored-by: Chris Peyer <[email protected]>
Co-authored-by: Santoshkumar Nateekar <[email protected]>
Co-authored-by: nateekar <[email protected]>
vgoodric pushed a commit to vgoodric/bootcamp-milo that referenced this pull request Feb 1, 2024
@yesil @vgoodric
MWPW-136866: merch-cards block (adobecom#1729)
9c074a5 
* MWPW-136866: merch-cards block (adobecom#1676)

* MWPW-136866: merch-cards block

* update code owners

* fix failing tests

* rename main to init

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

@honstar honstar honstar approved these changes

@npeltier npeltier npeltier approved these changes

Assignees
No one assigned
Labels
commerce needs-verification PR requires E2E testing by a reviewer new-feature New block or other feature run-nala Run Nala Test Automation against PR verified PR has been E2E tested by a reviewer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@yesil @npeltier @honstar @afmicka @skumar09