[PM-14422] Vault Carousel #12791
[PM-14422] Vault Carousel #12791
Conversation
- Some unique styles were not showing in storybook
…t/pm-14422/vault-carousel
merissaacosta
requested review from
vleague2
and removed request for
merissaacosta
|
Great job, no security vulnerabilities found in this Pull Request |
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #12791 +/- ##
==========================================
+ Coverage 34.34% 34.37% +0.02%
==========================================
Files 2965 2972 +7
Lines 90533 90603 +70
Branches 16977 16980 +3
==========================================
+ Hits 31094 31144 +50
- Misses 56975 56994 +19
- Partials 2464 2465 +1 ☔ View full report in Codecov by Sentry. |
|
This looks great! Something minor, the focus state in the recording differs slightly from what has been defined here. Could we match the Figma UI for consistency? Also is the click target at least 24px? |
…t/pm-14422/vault-carousel
| * Provide a height value of the tallest slide to prevent this. | ||
| * The value should be in `rem`. | ||
| */ | ||
| @Input() height?: `${number}rem` | undefined; |
There was a problem hiding this comment.
💭 I wonder if you can do this internally in the component? If you have access to all the slides (I haven't fully reviewed to check), you could grab the rendered element height of the tallest one and then set the height to that? (You might have to have all of them rendered but hidden to do that)
There was a problem hiding this comment.
💯 That was one of my ideas in the description. I think I'll have to render them off screen to avoid causing flashes to the user but I'll give it a shot!
There was a problem hiding this comment.
@sukhleenb Thank you for linking those! I didn't see those designs in the ticket.
Yes! Screen.Recording.2025-01-10.at.1.43.38.PM.mov |
…t/pm-14422/vault-carousel
| * Provide a height value of the tallest slide to prevent this. | ||
| * The value should be in `rem`. | ||
| */ | ||
| @Input() height?: `${number}rem` | undefined; |
There was a problem hiding this comment.
❓ should we have a default height if none is provided?
There was a problem hiding this comment.
I ended up calculating the height myself, see: #12791 (comment)
…t/pm-14422/vault-carousel
- This avoids consumers having to pass in a height. - The height of the tallest slide is needed because it will stop the carousel from jumping around as the user scrolls.
…t/pm-14422/vault-carousel
|
@vleague2 @gbubemismith you both pointed out the |
| }); | ||
|
|
||
| // Set the min height of the entire carousel based on the largest slide. | ||
| this.minHeight = `${(tallestSlideHeightPx + heightOfButtonsPx) / ROOT_PX_FONT_SIZE}rem`; |
There was a problem hiding this comment.
I think it is fine to leave this in pixels since it is dynamically calculated. I have some concerns about storing the root px font size in this file since it will likely get out of sync if we change the base font size in the css again, so using pixels for this minHeight means we don't need to do that.
|
|
||
| private _contentPortal: TemplatePortal | null = null; | ||
|
|
||
| get content(): TemplatePortal | null { |
There was a problem hiding this comment.
Not a blocker, but maybe add a comment here about what this is used for since it's not used directly in the template?
🎟️ Tracking
PM-14422
📔 Objective
Adds a Carousel components to Storybook under
Vault/Carousellibs/vault❓
There is a possibility that an implementation has differing heights across slides, which can result in the controls jumping around vertically. I added an input to provide a set height for the carousel so the controls can be pushed to the bottom.A couple other ideas:
carouselcomponent could render each slide invisible to the user and find the max height and then use that as a set height for all slides.📸 Screenshots
Screen.Recording.2025-01-09.at.2.46.38.PM.mov
carousel-screen-reader.mov
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes