[Doodlebug] New aside-carousel block#59
Conversation
|
Hello, I'm the AEM Code Sync Bot and I will run some actions to deploy your branch and validate page speed.
|
|
nkthakur48
requested review from
arugupta1992,
hkhatana26,
hkuraware,
suhjainadobe and
vipu0303
|
Hello @JingleH , |
|
Thankyou @JingleH for the fix on image peek view in left , when verified with video in ipad do not see it working as expected, can you please take a look |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| it('swipe left advances and swipe right goes back in RTL', () => { | ||
| const slides = rtlEl.querySelectorAll('.slide'); | ||
| const mkTouch = (x) => new Touch({ | ||
| identifier: 1, target: rtlEl, screenX: x, screenY: 0, clientX: x, clientY: 0, pageX: x, pageY: 0, |
Codecov Report❌ Patch coverage is
❌ Your patch status has failed because the patch coverage (89.67%) is below the target coverage (100.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## stage #59 +/- ##
==========================================
- Coverage 88.79% 87.73% -1.06%
==========================================
Files 42 42
Lines 9512 9442 -70
==========================================
- Hits 8446 8284 -162
- Misses 1066 1158 +92 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Hi @JingleH , verified the url after the fix, carousel arrow enablement is still not seen as expected |
|
|
||
| .aside-carousel .carousel-prev, | ||
| .aside-carousel .carousel-next { | ||
| background: #242424; |
There was a problem hiding this comment.
Several hardcoded hex values throughout the controls CSS have close or exact Milo equivalents: #242424 → --color-gray-800, #fff → --color-white, #e6e6e6 ≈ --color-gray-200, #d5d5d5 ≈ --color-gray-300, #b1b1b1 ≈ --color-gray-400. Using the tokens keeps the block consistent with the rest of the site.
There was a problem hiding this comment.
The goal was to preserve the same values as is currently being used in prod (milo's carousel). These are hardcoded the same as in Milo. I recommend us stick to the same visuals for now, and maybe involve the designers to sign off the change before working on getting the values consistent with other Milo's css tokens
| .aside-carousel .carousel-next:focus-visible { | ||
| background: #373737; | ||
| border-color: #fff; | ||
| outline: 2px solid #147af3; |
There was a problem hiding this comment.
This blue (#147af3) differs from Milo's focus ring token (--color-accent-focus-ring: #378ef0). Inconsistent focus indicators across the site can flag in a11y audits. Consider outline: 2px solid var(--color-accent-focus-ring, #378ef0);
There was a problem hiding this comment.
same as the other comment, the goal was to use the same visuals as Milo's carousel currently being used in prod
|
Please also see if the linter issues could be resolved |
|
Hi @nkthakur48 and @suhjainadobe , all PR comments should be resolved now. Can you review again? |
6 participants
Currently on Doodlebug pages, MEP is leveraged to swap out aside blocks with a section using carousel and editorial-cards. It could be better UX to use a responsive block (aside on desktop, carousel on tablet/mobile), and it would also remove the dependency on MEP, allowing automation of pages to be more streamlined.
This is a brand new block so should have no regression risk. No Figma yet as we are trying to proceed quickly. This is a best-effort attempt at combining multiple blocks and retaining doodlebug.css theme override under the time constraint.
Test URLs: