[CL-1115] accordion component#20508
Conversation
🤖 Bitwarden Claude Code ReviewOverall Assessment: APPROVE This PR introduces a new Code Review DetailsNo additional findings beyond the existing review thread by @vleague2. The author has addressed or responded to all raised concerns (storybook arg formatting, |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #20508 +/- ##
==========================================
- Coverage 47.11% 47.11% -0.01%
==========================================
Files 3951 3955 +4
Lines 119722 119839 +117
Branches 18346 18364 +18
==========================================
+ Hits 56407 56460 +53
- Misses 59081 59133 +52
- Partials 4234 4246 +12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| effect(() => { | ||
| const activeId = this.group?._activeAccordionId(); | ||
| if (activeId != null && activeId !== this._baseId) { | ||
| untracked(() => this.open.set(false)); |
There was a problem hiding this comment.
Why the untracked wrapper here?
There was a problem hiding this comment.
We are closing this accordion imperatively if the parent group component is single select and is controlling which accordion is open. We had to use untracked here because you can't write to a model inside of an effect AFAIK
| ].join(" "), | ||
| ); | ||
|
|
||
| private readonly triggerSizeClasses = computed((): string[] => { |
There was a problem hiding this comment.
There are other styles inline as ternaries; does this one need to be separate?
There was a problem hiding this comment.
Not necessarily. I can move it. Generally, I like the idea of all the styles being in the component vs the template. I guess I should just commit one way or the other :)
There was a problem hiding this comment.
For this one I think it is still used in the component side not the template. It's just split out into another function by itself
There was a problem hiding this comment.
Updated to be inline
There was a problem hiding this comment.
Yeah, I'm not sure why I did it that way.
| <bit-icon-tile [icon]="startIcon()" [size]="iconTileSize()" variant="contrast"></bit-icon-tile> | ||
| } | ||
| <div class="tw-flex tw-flex-col tw-flex-1 tw-min-w-0 tw-gap-1 tw-items-start"> | ||
| <span [class]="headingClassList()">{{ heading() }}</span> |
There was a problem hiding this comment.
Are there any typography variants that we can use for heading and subtitle?
There was a problem hiding this comment.
Yes and no. Because this switches text sizes and colors based on inactive state and the size variant passed in this felt easier to swap
|
2 participants
🎟️ Tracking
https://bitwarden.atlassian.net/browse/CL-1115
📔 Objective
Create new accordion component
📸 Screenshots
Variant: default
Size: default
Size: sm
Variant: subtle
Size: default
Size: sm