Skip to content

Conversation

@david-livefront
Copy link
Collaborator

@david-livefront david-livefront commented Oct 27, 2025

🎟️ Tracking

PM-27497

📔 Objective

This PR updates the Snackbar font for the message when there is no header present.

📸 Screenshots

Before After

⏰ 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

@claude
Copy link
Contributor

claude bot commented Oct 27, 2025

Claude finished @david-livefront's task —— View job


Code Review Complete

Tasks

  • Read repository guidelines and architecture documentation
  • Analyze PR changes in detail
  • Compile comprehensive review feedback

Summary of Changes

This PR updates the BitwardenSnackbar component to use titleSmall typography instead of bodyMedium when there is no header (messageHeader is null). This provides better visual hierarchy when the message appears standalone in the snackbar.

Changed File: ui/src/main/kotlin/com/bitwarden/ui/platform/components/snackbar/BitwardenSnackbar.kt:90-99


Critical Issues

None identified.


Suggested Improvements

1. Missing Test Coverage for the New Conditional Logic

Severity: Medium

The PR introduces conditional typography logic but doesn't include unit tests to verify the behavior. The component lacks dedicated test files, making it difficult to ensure the styling change works correctly in both scenarios.

Recommendation

Consider adding screenshot tests or snapshot tests to verify:

  • Message with header uses bodyMedium typography
  • Message without header uses titleSmall typography

This would prevent regressions and document the intended behavior. While UI module tests may be structured differently, having some form of automated verification would be valuable for this visual change.

2. Preview Function Could Demonstrate Both States

Severity: Low

The existing preview function BitwardenCustomSnackbar_preview() (line 127) only shows the snackbar with a header. It would be beneficial to add a second preview showing the snackbar without a header to visually demonstrate the change this PR addresses.

Example
@Preview
@Composable
private fun BitwardenCustomSnackbar_withoutHeader_preview() {
    BitwardenTheme {
        Surface {
            BitwardenSnackbar(
                BitwardenSnackbarData(
                    message = "Message without header".asText(),
                    actionLabel = "Action".asText(),
                    withDismissAction = true,
                ),
            )
        }
    }
}

This would make it easier for reviewers and designers to verify the visual changes.

3. Consider Documenting the Typography Decision

Severity: Low

The inline comment "Upgrade the font when it is stand alone" (line 96) is helpful, but it would be even better to document this design decision in the KDoc for the function or in a comment explaining the UX rationale (e.g., "standalone messages need more visual prominence").


Good Practices Observed

  • Uses conditional logic to maintain appropriate typography hierarchy
  • Preserves existing behavior when header is present
  • Inline comment explains the intent
  • Clean, readable implementation
  • Follows existing code style and formatting

Action Items for Author

  1. Optional but Recommended: Add a preview function demonstrating the snackbar without a header to showcase the visual change
  2. Optional: Consider adding tests for the conditional typography logic (if the project structure supports UI component tests)
  3. Verification: Ensure manual testing covers both scenarios:
    • Snackbar with header (should use bodyMedium)
    • Snackbar without header (should use titleSmall)

Architecture & Style Compliance

Architecture: Change is localized to the UI component and follows Compose best practices
Code Style: Follows Kotlin style guidelines and existing formatting patterns
Documentation: Inline comment provided (could be enhanced)
Best Practices: Uses appropriate conditional rendering with clear intent


Overall Assessment

This is a straightforward, well-implemented UI improvement that enhances the visual hierarchy of standalone snackbar messages. The change is minimal, focused, and follows existing patterns. The main suggestions are around adding test coverage and preview functions to prevent regressions and improve documentation.

Recommendation: Approve with optional improvements suggested above.


@david-livefront
Copy link
Collaborator Author

Thanks @SaintPatrck

@github-actions
Copy link
Contributor

Logo
Checkmarx One – Scan Summary & Details8108118e-ac53-46d7-8f9d-ddfd79d0de5f

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

@david-livefront david-livefront added this pull request to the merge queue Oct 27, 2025
@codecov
Copy link

codecov bot commented Oct 27, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 84.81%. Comparing base (c0f8307) to head (cd261e1).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff            @@
##             main    #6086    +/-   ##
========================================
  Coverage   84.81%   84.81%            
========================================
  Files         721      738    +17     
  Lines       52830    53193   +363     
  Branches     7671     7673     +2     
========================================
+ Hits        44809    45117   +308     
- Misses       5331     5386    +55     
  Partials     2690     2690            

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@david-livefront david-livefront changed the title PM-27136: Update Snackbar font when there is no header PM-27497: Update Snackbar font when there is no header Oct 27, 2025
Merged via the queue into main with commit 43d7b84 Oct 27, 2025
13 checks passed
@david-livefront david-livefront deleted the PM-27136-update-snackbar-font branch October 27, 2025 20:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants