Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CL-499][PM-14020] compact mode #11796

Merged
merged 28 commits into from
Nov 18, 2024
Merged

Conversation

willmartian
Copy link
Contributor

@willmartian willmartian commented Oct 30, 2024

🎟️ Tracking

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

https://bitwarden.atlassian.net/browse/PM-14020

📔 Objective

Adds a global compact mode setting to the DS for users who prefer a more information-dense UI. See "Compact Mode" in Storybook.

  • Adds a bit-compact: Tailwind variant and CompactModeService to hook into compact mode in CSS and TS, respectively.
  • Update various component templates to use the Tailwind variant.
    • The PopupLayoutComponent now owns the padding for the above-scroll-area slot
  • Updates the vault-items virtual scroll height to account for compact mode.
    • I moved ownership of the ItemHeight constant from the DS to Vault. We don't have a good way to guarantee the height of the component at the DS level, as it is based on the content passed to it. (This whole business of manually tracking heights for virtual scroll is very annoying. We will try to improve it in the future.)
  • Adds setting to Appearance page
Screen.Recording.2024-11-15.at.4.53.24.PM.mov

image

⏰ 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

Copy link

codecov bot commented Oct 30, 2024

Codecov Report

Attention: Patch coverage is 20.75472% with 42 lines in your changes missing coverage. Please review.

Project coverage is 33.48%. Comparing base (330057e) to head (3d87add).
Report is 4 commits behind head on main.

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
.../src/platform/popup/layout/popup-layout.stories.ts 0.00% 11 Missing ⚠️
...latform/popup/layout/popup-compact-mode.service.ts 45.45% 6 Missing ⚠️
...-container/vault-list-items-container.component.ts 0.00% 6 Missing ⚠️
libs/components/src/item/item.stories.ts 0.00% 6 Missing ⚠️
apps/browser/src/popup/app.component.ts 0.00% 3 Missing ⚠️
libs/components/src/item/item.component.ts 0.00% 3 Missing ⚠️
apps/browser/src/popup/services/services.module.ts 0.00% 2 Missing ⚠️
...rc/vault/popup/settings/appearance-v2.component.ts 75.00% 2 Missing ⚠️
.../components/src/form-field/form-field.component.ts 0.00% 0 Missing and 1 partial ⚠️
libs/components/src/index.ts 0.00% 1 Missing ⚠️
... and 1 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #11796      +/-   ##
==========================================
- Coverage   33.48%   33.48%   -0.01%     
==========================================
  Files        2846     2848       +2     
  Lines       89186    89225      +39     
  Branches    17000    17002       +2     
==========================================
+ Hits        29864    29873       +9     
- Misses      56969    56999      +30     
  Partials     2353     2353              

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


🚨 Try these New Features:

@willmartian willmartian requested a review from vleague2 October 30, 2024 20:20
libs/components/src/item/item.stories.ts Outdated Show resolved Hide resolved
libs/components/src/stories/compact-mode.mdx Outdated Show resolved Hide resolved
libs/components/src/shared/bit-settings.service.ts Outdated Show resolved Hide resolved
@vleague2
Copy link
Contributor

Not that this is caused by your PR, but I noticed that the item action content doesn't seem to be vertically centered. It looks more noticeable in compact mode to me
Screenshot 2024-10-31 at 9 53 06 AM

@willmartian
Copy link
Contributor Author

Not that this is caused by your PR, but I noticed that the item action content doesn't seem to be vertically centered. It looks more noticeable in compact mode to me Screenshot 2024-10-31 at 9 53 06 AM

Should be fixed with 9f05381

@willmartian willmartian marked this pull request as ready for review November 4, 2024 18:31
@willmartian willmartian requested a review from a team as a code owner November 4, 2024 18:31
@willmartian willmartian requested a review from vleague2 November 4, 2024 18:31

This comment was marked as off-topic.

Copy link
Member

@danielleflinn danielleflinn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Verified spacing and border-radius on bit-list and bit-list groups is expected
✅ Verified spacing is reduced for header and footer elements show as expected
✅ Verified footer elements are now aligned-center
✅ Known limitation of compact mode: when macOS has only show scrollbar "when scrolling" the scrollbar may overlap slightly with the bit-item content. This is acceptable as we are unable to adjust this with how the styling is currently handled.
⛔️ Unable to verify is navigation footer has reduced padding as shown here in figma

@vleague2
Copy link
Contributor

vleague2 commented Nov 6, 2024

I just remembered that we have some hardcoded heights in px for virtual scrolling. We'll probably need compact mode versions of that, right? Otherwise it won't work in virtual scroll?

@willmartian
Copy link
Contributor Author

@vleague2 I just remembered that we have some hardcoded heights in px for virtual scrolling. We'll probably need compact mode versions of that, right? Otherwise it won't work in virtual scroll?

Yes. Great call out. I need to think about the best way to handle that.

@willmartian willmartian requested review from a team as code owners November 12, 2024 21:28
@willmartian willmartian marked this pull request as draft November 12, 2024 22:15
@willmartian willmartian changed the base branch from ps/extension-refresh to main November 12, 2024 22:19
@willmartian willmartian changed the base branch from main to ps/extension-refresh November 12, 2024 22:19
@willmartian willmartian changed the title [CL-499] compact mode [CL-499][PM-14020] compact mode Nov 13, 2024
@willmartian willmartian marked this pull request as ready for review November 13, 2024 18:05
@djsmith85 djsmith85 self-requested a review November 13, 2024 18:20
danielleflinn
danielleflinn previously approved these changes Nov 13, 2024
Copy link
Member

@danielleflinn danielleflinn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ bit-item-group doesn't have the extra 6px of space at the end of the list in either modes that was previously observed
✅ bit-item spacing updated + no spacing exists between list items and last item retains the bottom border
✅ bit-item has a min-height that matches button height for instances that only have 1 line of text
✅ bit-items instances in the extension previously observed to have a missing top border radius now show the proper radius
✅ extension layout now handles search slot padding allowing it to be updated in compact mode. Verified padding is expected in compact mode on Vault and Send pages
✅ Verified no unexpected bottom padding on Vault virtual scroll container when searching or filtering
✅ Compact mode setting appears on Appearance page and includes a beta badge. Setting persists when user closes the extension
❓ Main navigation padding still matches the default padding. Ideally we would drop this top padding by about 4px to more closely match the height of the footer with buttons (view/edit pages). That said, I missed this in my last list of feedback so I'm ok with us moving this to followup work so we can move the feature to QA

@willmartian
Copy link
Contributor Author

willmartian commented Nov 14, 2024

❓ Main navigation padding still matches the default padding. Ideally we would drop this top padding by about 4px to more closely match the height of the footer with buttons (view/edit pages). That said, I missed this in my last list of feedback so I'm ok with us moving this to followup work so we can move the feature to QA

@danielleflinn Oops, added here: 1296019

Screen.Recording.2024-11-13.at.9.01.49.PM.mov

shane-melton
shane-melton previously approved these changes Nov 14, 2024
Copy link
Member

@shane-melton shane-melton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Vault changes look good!

@willmartian willmartian force-pushed the ds/cl-499/item-group-compact-mode branch from 1296019 to adbfe0c Compare November 15, 2024 21:06
@willmartian
Copy link
Contributor Author

willmartian commented Nov 15, 2024

Sorry, I borked the commit history with a bad rebase after merging the feature branch. The above commits reflect what was already reviewed and approved by all code owners except Tools. The below commits are new (DS only).

@willmartian willmartian marked this pull request as ready for review November 15, 2024 21:54
danielleflinn
danielleflinn previously approved these changes Nov 15, 2024
Copy link
Member

@danielleflinn danielleflinn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Verified page header and footer spacing in default mode was updated to match requirements
✅ Verified compact mode card, form-field, and sections, now have decreased padding

djsmith85
djsmith85 previously approved these changes Nov 18, 2024
Copy link
Contributor

@djsmith85 djsmith85 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving for tools

Copy link
Contributor

@vleague2 vleague2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thoughts/q's but looking really good!

vleague2
vleague2 previously approved these changes Nov 18, 2024
@willmartian willmartian merged commit a07b072 into main Nov 18, 2024
72 of 73 checks passed
@willmartian willmartian deleted the ds/cl-499/item-group-compact-mode branch November 18, 2024 21:35
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.

8 participants