feature(case-study-carousel): Case study carousel block. #323
Conversation
nimithshetty17
requested review from
proeung,
keith-kaplan,
sachinmesh,
sharathmrft and
sahmad-merative
|
Hello, I'm Franklin Bot and I will run some test suites that validate the page speed.
|
|
nimithshetty17
added
the
Needs design QA
There was a problem hiding this comment.
1. In desktop, eyebrow font style should be 14px Alliance No.1 medium and not 16px.
2. Title Text box should be 268px wide
3. Body text box should be 446px wide.
5. Spacing between side arrows with above and below componenets should be 48px.
6. Carousal indicators spacing with carousal card should be 64px.
|
|
@sachinmesh - The requested changes have been committed. Please have a look. |
|
Checking the link here. HI @nimithshetty17 Changes pending are
|
|
@sachinmesh I have cleared the cache. Please check in an incognito window now. |
|
CHecked @nimithshetty17 Rest is good, but the carousal arrows are gone, can you check once. 
|
|
|
@sachinmesh have added the arrows back. |
|
Good to go @nimithshetty17 @proeung @sharathmrft |
|
sachinmesh
removed
the
Needs design QA
|
|
|
@proeung @sachinmesh have done the requested changes. Please have a look. |
|
Good to go now with 100 x 100 carousal arrow. |
sachinmesh
added
Ready to merge
proeung
removed
the
Ready to merge
There was a problem hiding this comment.
@nimithshetty17 Thanks for addressing my code suggestions and feedback.
Overall the Carousel - Case Study block variation looks great, however, I'm hesitant to merge these change down since, the Testimonial carousel no longer rendering correctly (see attached).
@helms-charity Since you worked on the Testimonial carousel, can you help take a look at why it's rendering. Perhaps, there's something that's missing within the Sharepoint doc.
https://case-study-carousel--merative2--nimithshetty17.hlx.page/block-library/blocks/carousel
|
@nimithshetty17 @proeung ".carousel-slide-container" is not getting the calculated height, so it is presenting at 0 height. (when you put some number in the browser console, the content shows). I'll see if I can find what happened.
|
| font-family: var(--serif-font); | ||
| font-weight: var(--font-weight-light); | ||
| line-height: 124%; | ||
| font-size: 31px; |
There was a problem hiding this comment.
The function "calculateSlideHeight" seems to need a font size as a number, not a variable in order to render the .carousel-slide-container. So you can put this back but specify the .carousel.testimonial type?
There was a problem hiding this comment.
Also, I remember I tried using 32px as the font-size here, but there's something funky with your custome font, and I had to tell the JS to use 31px instead of 32px so that nothing would get cut off.
|
|
@proeung - Testimonal carousel issue is fixed now. |
proeung
added
Ready to merge
Issue
Fixes #MERATIVE-785
Description
There's already a block in Franklin called "Carousel". This will be a variation called "Carousel - Case Study".
New
Design Specs
Test URLs
main): https://main--merative2--nimithshetty17.hlx.page/block-library/blocks/carouselTesting Instruction
To test the case study carousel and its responsiveness with respect to the design across all devices.