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

[PM-7611] Create a new vault settings component [UI changes] #9220

Merged

Conversation

djsmith85
Copy link
Contributor

@djsmith85 djsmith85 commented May 16, 2024

Type of change

- [ ] Bug fix
- [X] New feature development
- [ ] Tech debt (refactoring, code cleanup, dependency upgrades, etc)
- [ ] Build/deploy pipeline (DevOps)
- [ ] Other

Objective

With #8840 the navigational changes were made. This is meant to update UI for the extensions refresh according to the new design.

The currently missing trash anchor and page will be added with PM-8125

Code changes

  • Create vault-settings-v2 component - b8b8067**
    • Copy existing vault-settings component
    • Make new component standalone
    • Replace app-header with popup-header
    • Replace nav-buttons with bit-item
    • Register route to show new component when extension refresh flag is enabled
  • Move sync functionality to vault-settings-v2 - 55db865
    • Enables syncing straight on the vault-settings page instead of going to another sub-page
    • sync.component will need to be deleted when we fully switch over to the new design
  • Use anchors instead of buttons when navigating to a sub-page - 537eaa6
    • Better accessibility

Screenshots

Before

before_vault-settings

After

after_vault-settings

Before you submit

  • Please add unit tests where it makes sense to do so (encouraged but not required)
  • If this change requires a documentation update - notify the documentation team
  • If this change has particular deployment requirements - notify the DevOps team
  • Ensure that all UI additions follow WCAG AA requirements

- Copy existing vault-settings component
- Make new component standalone
- Replace app-header with popup-header
- Replace nav-buttons with bit-item

- Register route to show new component when extension refresh flag is enabled
@djsmith85 djsmith85 requested a review from a team as a code owner May 16, 2024 18:33
@djsmith85 djsmith85 requested a review from Jingo88 May 16, 2024 18:33
@github-actions github-actions bot added the needs-qa Marks a PR as requiring QA approval label May 16, 2024
…s/pm-7611/create-a-new-vault-settings-component-UI-changes
Copy link

codecov bot commented May 16, 2024

Codecov Report

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

Project coverage is 28.13%. Comparing base (56c4be4) to head (490ab8b).

Files Patch % Lines
...ault/popup/settings/vault-settings-v2.component.ts 0.00% 33 Missing ⚠️
apps/browser/src/popup/app-routing.module.ts 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9220      +/-   ##
==========================================
- Coverage   28.14%   28.13%   -0.02%     
==========================================
  Files        2361     2362       +1     
  Lines       69846    69880      +34     
  Branches    13133    13139       +6     
==========================================
  Hits        19660    19660              
- Misses      48629    48663      +34     
  Partials     1557     1557              

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

Copy link
Contributor

github-actions bot commented May 16, 2024

Logo
Checkmarx One – Scan Summary & Details9dc26f40-c24a-4dc6-8ad4-251a16b7ecde

New Issues

Severity Issue Source File / Package Checkmarx Insight
MEDIUM Missing_HSTS_Header /libs/common/src/services/api.service.ts: 222 Attack Vector

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.

Looks good! Just a few minor suggestions.

apps/browser/src/popup/app-routing.module.ts Outdated Show resolved Hide resolved
…s/pm-7611/create-a-new-vault-settings-component-UI-changes
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.

Thank you!

…s/pm-7611/create-a-new-vault-settings-component-UI-changes
…s/pm-7611/create-a-new-vault-settings-component-UI-changes
@djsmith85 djsmith85 removed the needs-qa Marks a PR as requiring QA approval label May 21, 2024
@djsmith85 djsmith85 enabled auto-merge (squash) May 21, 2024 20:13
@djsmith85 djsmith85 merged commit 983a82c into main May 21, 2024
17 of 18 checks passed
@djsmith85 djsmith85 deleted the tools/pm-7611/create-a-new-vault-settings-component-UI-changes branch May 21, 2024 20:13
quexten pushed a commit that referenced this pull request May 22, 2024
* Create vault-settings-v2 component

- Copy existing vault-settings component
- Make new component standalone
- Replace app-header with popup-header
- Replace nav-buttons with bit-item

- Register route to show new component when extension refresh flag is enabled

* Move sync functionality to vault-settings-v2

* Use anchors instead of buttons when navigating to a sub-page

* Removed unneeded component within routing

* Use new ToastService instead of PlatformUtils

* Remove unused MessagingService

---------

Co-authored-by: Daniel James Smith <djsmith85@users.noreply.github.com>
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.

None yet

3 participants