[CL-947] drawer width variants

[BryanCunningham]

🎟️ Tracking

https://bitwarden.atlassian.net/browse/CL-947

📔 Objective

Adds ability to configure drawer size

📸 Screenshots

small

default

large

⏰ Reminders before review

• Contributor guidelines followed
• All formatters and local linters executed and passed
• Written new unit and / or integration tests where applicable
• Protected functional changes with optionality (feature flags)
• Used internationalization (i18n) for all UI strings
• CI builds passed
• Communicated to DevOps any deployment requirements
• Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team

🦮 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

[github-actions]

Checkmarx One – Scan Summary & Details – edcf32cf-3621-4362-8628-8707fb0e25c1

Great job! No new security vulnerabilities introduced in this pull request

[codecov]

Codecov Report

❌ Patch coverage is 6.06061% with 31 lines in your changes missing coverage. Please review.
✅ Project coverage is 42.28%. Comparing base (c6b0208) to head (72a78c7).
⚠️ Report is 3 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
...bs/components/src/dialog/dialog.service.stories.ts	0.00%	20 Missing ⚠️
...s/components/src/dialog/dialog/dialog.component.ts	15.38%	11 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #18156      +/-   ##
==========================================
+ Coverage   42.27%   42.28%   +0.01%     
==========================================
  Files        3599     3599              
  Lines      104548   104577      +29     
  Branches    15780    15787       +7     
==========================================
+ Hits        44194    44218      +24     
- Misses      58473    58485      +12     
+ Partials     1881     1874       -7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[BryanCunningham]

@claude review this PR

[claude]

Claude finished @BryanCunningham's task in 1m 26s —— View job

---

PR Review for Drawer Width Variants

Reviewing changes for: CL-947 drawer width variants

Todo List

• Read and analyze changed files
• Review code quality and best practices
• Check test coverage concerns
• Provide comprehensive feedback

---

Summary

This PR successfully adds configurable drawer size variants (small, default, large) for the dialog system. The implementation is clean and follows good separation of concerns by extracting the width logic into a dedicated utility function. However, there are a few areas that need attention:

Key Changes

1. New file: get-dialog-width-styles.ts - Utility function to map dialog/drawer sizes to Tailwind classes
2. Updated: dialog.component.ts - Refactored to use the new utility function
3. Updated: dialog.service.stories.ts - Added Storybook examples for small and large drawer variants

---

Issues & Recommendations

⚠️ Critical: Potential Runtime Bug

File: libs/components/src/dialog/get-dialog-width-styles.ts:16

export const getDialogWidthStyles = (size: DialogSize = "default", isDrawer: boolean = false) =>
  isDrawer ? drawerSizeToWidthMap.get(size) : dialogSizeToWidthMap.get(size);

Issue: The function can return undefined if an invalid size is passed (or if the Map doesn't contain the key, though that's unlikely with the current setup). The return type should be string | undefined or the function should handle undefined.

Recommendation: Either:

1. Add explicit return type and handle undefined:

export const getDialogWidthStyles = (
  size: DialogSize = "default", 
  isDrawer: boolean = false
): string => {
  const map = isDrawer ? drawerSizeToWidthMap : dialogSizeToWidthMap;
  return map.get(size) ?? map.get("default")!;
};

2. Or document that undefined is possible and handle it in the component.

---

🎨 Code Quality: Use const object instead of Map

File: libs/components/src/dialog/get-dialog-width-styles.ts:3-13

Current approach:

const dialogSizeToWidthMap = new Map<DialogSize, string>([
  ["small", "md:tw-max-w-sm"],
  ["default", "md:tw-max-w-xl"],
  ["large", "md:tw-max-w-3xl"],
]);

Issue: Using Map is unnecessarily complex for a static mapping. A const object would be more idiomatic and performant.

Recommendation: Following Bitwarden's pattern (similar to ADR-0025 enum replacement pattern):

const dialogSizeToWidth = {
  small: "md:tw-max-w-sm",
  default: "md:tw-max-w-xl",
  large: "md:tw-max-w-3xl",
} as const;

const drawerSizeToWidth = {
  small: "md:tw-max-w-sm",
  default: "md:tw-max-w-lg",
  large: "md:tw-max-w-2xl",
} as const;

export const getDialogWidthStyles = (size: DialogSize = "default", isDrawer: boolean = false) =>
  isDrawer ? drawerSizeToWidth[size] : dialogSizeToWidth[size];

This is more performant (no Map overhead), more TypeScript-idiomatic, and provides better type safety.

---

📝 Test Coverage

Issue: Codecov reports only 11.11% patch coverage with 24 lines missing coverage:

• dialog.service.stories.ts: 20 missing lines (Storybook stories - typically not covered)
• dialog.component.ts: 2 missing lines
• get-dialog-width-styles.ts: 2 missing lines

Observations:

1. The Storybook stories (20 lines) are expected to be uncovered - this is normal
2. The new utility function get-dialog-width-styles.ts lacks unit tests
3. The dialog component changes aren't fully covered

Recommendation:

• Add unit tests for getDialogWidthStyles function covering:
  • Dialog mode (small, default, large)
  • Drawer mode (small, default, large)
  • Default parameter behavior

Example test structure:

describe("getDialogWidthStyles", () => {
  describe("dialog mode", () => {
    it("returns correct class for small dialog", () => {
      expect(getDialogWidthStyles("small", false)).toBe("md:tw-max-w-sm");
    });
    // ... other sizes
  });
  
  describe("drawer mode", () => {
    it("returns correct class for small drawer", () => {
      expect(getDialogWidthStyles("small", true)).toBe("md:tw-max-w-sm");
    });
    // ... other sizes
  });
});

---

🤔 Drawer Width Change

File: libs/components/src/dialog/dialog/dialog.component.ts:108

Change:

// Before:
? ["tw-h-full", "md:tw-w-[23rem]"]
// After:
? ["tw-size-full"]

Observation: The previous implementation used a fixed width of 23rem (368px) for drawers at medium+ screens. The new implementation:

• Uses tw-size-full which sets both width AND height to 100%
• The width is now controlled separately via the width getter which uses getDialogWidthStyles()

Questions:

1. Is there a reason for removing the explicit md:tw-w-[23rem]?
2. The old default was 23rem (~368px), the new default is md:tw-max-w-lg (512px) - this is a ~39% increase. Was this intentional?
3. Should this breaking change be documented for existing drawer users?

---

👍 What's Good

1. Good separation of concerns: Extracting the width logic into a utility function makes it reusable and testable
2. Type safety: Using DialogSize type alias provides good type safety
3. Storybook examples: Adding visual examples for all size variants is excellent for documentation
4. Consistent naming: The size variants (small, default, large) are intuitive and consistent
5. Tailwind prefix compliance: All classes use the tw- prefix as required by Bitwarden standards

---

Minor Observations

📝 Export Considerations

File: libs/components/src/dialog/get-dialog-width-styles.ts

The DialogSize type and getDialogWidthStyles function are exported, but consider whether they should be re-exported through a barrel export (index.ts) for easier consumption by other components. Check if there's a pattern in the dialog module for this.

---

Checklist Review

Looking at your PR checklist:

• ✅ Screenshots provided showing all three size variants
• ❌ Test coverage needs attention (only 11.11% patch coverage)
• ✅ Uses i18n appropriately (no new strings added)
• ✅ Tailwind classes use tw- prefix
• ⚠️ Consider if this width change requires documentation updates

---

Recommendation

The core functionality looks solid, but please address:

1. Critical: Handle potential undefined return from getDialogWidthStyles
2. Recommended: Replace Map with const objects for better performance and idiomaticity
3. Recommended: Add unit tests for the utility function
4. Clarification needed: Confirm the drawer width change from 23rem to lg (512px) is intentional

Overall, this is a well-structured PR with a clean implementation. The main concerns are around test coverage and the potential undefined return value. Great job on the Storybook examples! 👍

---

[willmartian]

Just a few things:

[BryanCunningham]

@claude review this again

[claude]

Claude encountered an error —— View job

---

I'll analyze this and get back to you.

[claude]

Claude finished @BryanCunningham's task in 1m 32s —— View job

---

Re-reviewing PR Changes

Reviewing: CL-947 drawer width variants

Todo List

• Read current state of changed files
• Compare against previous review feedback
• Evaluate @willmartian's review comments
• Provide comprehensive updated feedback

---

Summary

Excellent progress! The author has addressed most of the concerns from the previous review. The implementation now uses const objects instead of Maps, inlines the width logic into a computed signal, and properly handles drawer sizing. However, @willmartian has identified some valid concerns about class duplication that should be addressed.

---

What Was Fixed ✅

1. ✅ Map → const object: Changed from Map<DialogSize, string> to const objects (dialogSizeToWidth and drawerSizeToWidth). Great improvement!

2. ✅ Computed signal: The width getter was converted to a computed signal as suggested by @willmartian in his review (dialog.component.ts:117-126)

3. ✅ Inlined logic: The separate utility file (get-dialog-width-styles.ts) was removed and the logic was inlined into the component, which is cleaner for this use case

4. ✅ Removed fixed drawer width: The old hardcoded md:tw-w-[23rem] was removed in favor of the configurable width system

5. ✅ Type alias: Used DialogSize type alias for better type safety

---

Remaining Issues

⚠️ Critical: Duplicate tw-w-screen class (@willmartian's feedback)

Location: dialog.component.ts:130 and line 6 in dialog.component.html

Issue: As @willmartian correctly identified, we have tw-w-screen appearing in multiple places:

1. In baseClasses array (line 130): ["tw-flex", "tw-flex-col", "tw-w-screen"]
2. Duplicated in the old sizeClasses for non-drawer dialogs (removed but was problematic)
3. The width is now being set via the width() computed signal which returns values like md:tw-max-w-lg

Current behavior:

• For drawers: tw-w-screen (from baseClasses) + tw-h-full (from sizeClasses) + width like md:tw-max-w-lg (from width computed)
• For dialogs: tw-w-screen (from baseClasses) + md:tw-p-4 + tw-max-h-[90vh] (from sizeClasses) + width like md:tw-max-w-xl (from width computed)

Problem: The tw-w-screen in baseClasses conflicts with the responsive max-width classes from the width() computed signal. At mobile sizes this is fine (both want full width), but at md: breakpoint and above, we're saying both "be full screen width" AND "have a max width of X". The max-width wins, but it's confusing and redundant.

Recommendation: Remove tw-w-screen from baseClasses since the width is now controlled by the width() computed signal. For mobile, the component will naturally take full width of its container without tw-w-screen, and at md: breakpoint, the max-width classes will apply.

Suggested fix (dialog.component.ts:130):

const baseClasses = ["tw-flex", "tw-flex-col"];

The tw-w-screen is unnecessary because:

1. For drawers: They're already in a full-height container and will take full width naturally
2. For dialogs: The md:tw-max-w-* classes from width() control the width at medium+ screens, and block-level elements take full width by default on mobile

---

📝 Minor: Template uses width() correctly

Location: dialog.component.html:5

The template correctly applies the width via:

[ngClass]="[
  width(),
  isDrawer ? 'tw-h-full tw-border-t-0' : 'tw-rounded-t-xl md:tw-rounded-xl tw-shadow-lg',
]"

This is good - the width is being applied to the inner <section> element, not the outer host element. This is the correct approach.

---

📝 Test Coverage Still Needs Attention

The test coverage concern from the previous review still applies:

• No unit tests for the new width logic
• The computed signal logic (width calculation based on size and drawer mode) should be tested

Recommendation: Add unit tests for the DialogComponent covering:

1. Width classes for each size in dialog mode
2. Width classes for each size in drawer mode
3. Verify the correct const object is used based on drawer vs dialog

Example test structure:

describe("DialogComponent width", () => {
  it("should use dialog widths for dialog mode", () => {
    const component = createComponent({ dialogSize: "small", isDrawer: false });
    expect(component.width()).toBe("md:tw-max-w-sm");
  });

  it("should use drawer widths for drawer mode", () => {
    const component = createComponent({ dialogSize: "default", isDrawer: true });
    expect(component.width()).toBe("md:tw-max-w-lg");
  });
  
  // ... test all size variants
});

---

What's Great 👍

1. Computed signal approach: Using a computed signal for width is the correct Angular pattern and integrates well with the signal-based inputs

2. Const objects: The const objects with as const provide good type safety and are more performant than Maps

3. Clean separation: Dialog and drawer widths are clearly separated into their own const objects

4. Storybook examples: The stories file now has examples for small and large drawer variants, which is great for documentation and visual testing

5. Type safety: The DialogSize type alias ensures only valid sizes can be used

---

Minor Observations

💭 Width Change Impact

The default drawer width changed from 23rem (~368px) to md:tw-max-w-lg (512px) - this is a ~39% increase. Based on the screenshots provided in the PR description, this looks intentional and good. The wider default provides more breathing room for drawer content.

---

Recommendations Summary

Must fix:

1. ⚠️ Remove tw-w-screen from baseClasses array (line 130) to eliminate class duplication

Should add:
2. 📝 Add unit tests for the width computation logic

Nice to have:
3. Consider exporting DialogSize type from a barrel export if it needs to be used elsewhere

---

Suggested Change

Here's the one-line fix for the duplicate tw-w-screen issue:

File: libs/components/src/dialog/dialog/dialog.component.ts:130

// Change from:
const baseClasses = ["tw-flex", "tw-flex-col", "tw-w-screen"];

// To:
const baseClasses = ["tw-flex", "tw-flex-col"];

---

Overall, this PR is in much better shape! The main issue is the class duplication that @willmartian identified. Once that's fixed and tests are added, this will be ready to merge. Great work on addressing the previous feedback! 🎉

[willmartian]

My previous comments were addressed, thank you, but it looks like there is an issue with this story: https://62a88a6de5b807fa98886113-dgzwhyfrul.chromatic.com/?path=/docs/web-organizations-access-selector-dialog--docs

[BryanCunningham]

My previous comments were addressed, thank you, but it looks like there is an issue with this story: https://62a88a6de5b807fa98886113-dgzwhyfrul.chromatic.com/?path=/docs/web-organizations-access-selector-dialog--docs

@willmartian dialogSize is undefined in this case. I put some code in the component to make sure if undefined is passed in we have no rendering errors