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-500] Add disclosure component and directive #11865

Merged
merged 5 commits into from
Nov 7, 2024

Conversation

vleague2
Copy link
Contributor

@vleague2 vleague2 commented Nov 5, 2024

🎟️ Tracking

CL-500

📔 Objective

This PR adds a disclosure component and accompanying directive that controls the disclosure's visibility. It also adds icon-button styling to handle the appearance of the muted variant button when its disclosure area is expanded. (This functionality is not behind a feature flag, but will only affect icon buttons used with the disclosure.)

Changes to the icon-button documentation files are remnants of a prior approach, but they still improve the documentation so I'm leaving them in here.

📸 Screenshots

Screen.Recording.2024-11-06.at.10.44.06.AM.mov

⏰ 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 Nov 5, 2024

Codecov Report

Attention: Patch coverage is 0% with 21 lines in your changes missing coverage. Please review.

Project coverage is 33.59%. Comparing base (f909855) to head (c1f74b2).
Report is 2 commits behind head on main.

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...bs/components/src/disclosure/disclosure.stories.ts 0.00% 7 Missing ⚠️
.../components/src/disclosure/disclosure.component.ts 0.00% 6 Missing ⚠️
...src/disclosure/disclosure-trigger-for.directive.ts 0.00% 5 Missing ⚠️
libs/components/src/disclosure/index.ts 0.00% 2 Missing ⚠️
libs/components/src/index.ts 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #11865      +/-   ##
==========================================
- Coverage   33.59%   33.59%   -0.01%     
==========================================
  Files        2825     2829       +4     
  Lines       88289    88310      +21     
  Branches    16834    16835       +1     
==========================================
+ Hits        29658    29665       +7     
- Misses      56308    56322      +14     
  Partials     2323     2323              

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

@vleague2 vleague2 changed the title [CL-500] Add toggle functionality for icon button [CL-500] Add disclosure component and directive Nov 6, 2024
@vleague2 vleague2 marked this pull request as ready for review November 6, 2024 15:46
@vleague2 vleague2 requested a review from a team as a code owner November 6, 2024 15:46
@vleague2 vleague2 requested review from merissaacosta and willmartian and removed request for merissaacosta November 6, 2024 15:46
@danielleflinn danielleflinn self-requested a review November 6, 2024 16:30
danielleflinn
danielleflinn previously approved these changes Nov 6, 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 default, hover, selected, and focus styles match Figma
✅ Verified SR announcements with VO in Chrome match Figma
✅ Verified on click content his shown/hidden

Should we call out in the Disclosure docs "Accessibility" section to be sure to still follow the accessibility guidelines for the button/icon-button triggering the element? (I'm specifically thinking of the icon-button here hoping folks wont' forget to use the appA11yTitle)

willmartian
willmartian previously approved these changes Nov 7, 2024
Copy link
Contributor

@willmartian willmartian left a comment

Choose a reason for hiding this comment

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

This looks great!

@vleague2 vleague2 requested review from a team as code owners November 7, 2024 20:17
@vleague2 vleague2 changed the base branch from ps/extension-refresh to main November 7, 2024 20:17
@vleague2 vleague2 dismissed stale reviews from willmartian and danielleflinn November 7, 2024 20:17

The base branch was changed.

@vleague2 vleague2 removed request for a team and cd-bitwarden November 7, 2024 20:17
@vleague2
Copy link
Contributor Author

vleague2 commented Nov 7, 2024

Rebased to main so that it can be used on the Vault page prior to the extension refresh feature branch being merged

Copy link
Contributor

github-actions bot commented Nov 7, 2024

Logo
Checkmarx One – Scan Summary & Details58cda4f8-7afc-490c-8905-ed9592598d96

Fixed Issues

Severity Issue Source File / Package
MEDIUM Client_Privacy_Violation /apps/web/src/app/tools/risk-insights/password-health-members.component.html: 55
MEDIUM Client_Privacy_Violation /apps/web/src/app/tools/risk-insights/password-health-members.component.html: 50
MEDIUM Client_Privacy_Violation /apps/web/src/app/tools/risk-insights/password-health-members.component.html: 45
MEDIUM Client_Privacy_Violation /apps/web/src/app/tools/risk-insights/password-health-members.component.html: 50
MEDIUM Client_Privacy_Violation /apps/web/src/app/tools/risk-insights/password-health-members-uri.component.html: 40
MEDIUM Client_Privacy_Violation /apps/web/src/app/tools/risk-insights/password-health-members-uri.component.html: 40
MEDIUM Client_Privacy_Violation /apps/web/src/app/tools/risk-insights/password-health-members-uri.component.html: 35
MEDIUM Client_Privacy_Violation /apps/web/src/app/tools/risk-insights/password-health-members-uri.component.html: 45
MEDIUM Client_Privacy_Violation /apps/web/src/app/tools/risk-insights/password-health.component.html: 45
MEDIUM Client_Privacy_Violation /apps/web/src/app/tools/risk-insights/password-health.component.html: 40
MEDIUM Client_Privacy_Violation /apps/web/src/app/tools/risk-insights/password-health.component.html: 50
MEDIUM Client_Privacy_Violation /apps/web/src/app/tools/risk-insights/password-health.component.html: 45
MEDIUM Client_Privacy_Violation /libs/vault/src/components/totp-countdown/totp-countdown.component.ts: 14
MEDIUM Client_Privacy_Violation /libs/vault/src/components/totp-countdown/totp-countdown.component.ts: 15
MEDIUM Client_Privacy_Violation /libs/tools/generator/components/src/username-generator.component.html: 3
LOW Angular_Usage_of_Unsafe_DOM_Sanitizer /libs/components/src/avatar/avatar.component.ts: 80
LOW Angular_Usage_of_Unsafe_DOM_Sanitizer /apps/desktop/src/app/components/avatar.component.ts: 75
LOW Client_Hardcoded_Domain /apps/web/src/app/billing/shared/payment/payment.component.ts: 73
LOW Client_Hardcoded_Domain /apps/web/src/app/billing/shared/payment/payment.component.ts: 73

@vleague2 vleague2 merged commit e8dac0c into main Nov 7, 2024
70 of 71 checks passed
@vleague2 vleague2 deleted the ds/cl-500/icon-button-toggle branch November 7, 2024 21:54
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