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.

Sign up for GitHub

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

Jump to bottom

[SM-956] Secret Manager: Integrations Page #8701

Merged
merged 23 commits into from
Apr 18, 2024
Merged

[SM-956] Secret Manager: Integrations Page #8701

merged 23 commits into from
Apr 18, 2024

Conversation

nick-livefront
Copy link
Collaborator

@nick-livefront nick-livefront commented Apr 11, 2024 •
edited
Loading

Type of change

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

Objective

Add Integrations page within Secrets manager than links to internal integrations or SDKs available on GitHub.

  • I have an open question about which SDKs should be marked as "new" in the ticket. This should be an easy add as a fast follow or tack on to this PR when I receive an answer.

Code changes

  • messages.json: Added needed copy
  • images/secret-manager/*: Added all language & integration images within a secrets-manager/ folder
  • integration-card/*: Card component to link to an Integration or SDK
    • I used a pseudo element to cover the entire card as clickable but the link will still only be accessible via keyboard/screen reader rather than the entire card.
    • The new badge is driven by a client side time comparison. This could be done manually or by other means!
  • integration-grid/*: Arranges individual cards into a grid based on the designs
  • integrations/*: Markup & constant data for the Integrations page

Screenshots

image

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

@github-actions github-actions bot added the needs-qa Marks a PR as requiring QA approval label Apr 11, 2024
@codecov Codecov
Copy link

codecov bot commented Apr 11, 2024 •
edited
Loading

Codecov Report

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

Project coverage is 27.36%. Comparing base (1e0ad09) to head (097d5b9).

Files Patch % Lines
...ecrets-manager/integrations/integrations.module.ts 0.00% 7 Missing ⚠️
...anager/integrations/integrations-routing.module.ts 0.00% 5 Missing ⚠️
...t-web/src/app/secrets-manager/sm-routing.module.ts 0.00% 2 Missing ⚠️
...ons/integration-card/integration-card.component.ts 96.15% 0 Missing and 1 partial ⚠️
...ets-manager/integrations/integrations.component.ts 90.90% 0 Missing and 1 partial ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #8701      +/-   ##
==========================================
+ Coverage   27.32%   27.36%   +0.03%     
==========================================
  Files        2349     2355       +6     
  Lines       68609    68668      +59     
  Branches    12827    12835       +8     
==========================================
+ Hits        18748    18791      +43     
- Misses      48451    48465      +14     
- Partials     1410     1412       +2

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

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Apr 11, 2024 •
edited
Loading

Logo
Checkmarx One – Scan Summary & Details – f5d07e01-715b-45ac-9401-ec2869e5ac29

No New Or Fixed Issues Found

@nick-livefront nick-livefront marked this pull request as ready for review April 12, 2024 13:56
@nick-livefront nick-livefront requested a review from a team as a code owner April 12, 2024 13:56
differsthecat
differsthecat reviewed Apr 12, 2024
View reviewed changes
Copy link
Member

@differsthecat differsthecat 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 really good, thank you @nick-livefront !
I only have a few small feedback items 🙂

apps/web/src/locales/en/messages.json Outdated Show resolved Hide resolved
apps/web/src/images/secrets-manager/integrations/github.svg Outdated Show resolved Hide resolved
...web/src/app/secrets-manager/integrations/integration-card/integration-card.component.spec.ts Outdated Show resolved Hide resolved
@differsthecat differsthecat requested a review from a team April 12, 2024 20:26
cd-bitwarden
cd-bitwarden requested changes Apr 17, 2024
View reviewed changes
Copy link
Contributor

@cd-bitwarden cd-bitwarden 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 really great! I just left one suggestion, and I'm waiting to see that the Github dark mode image is added :)

...it-web/src/app/secrets-manager/integrations/integration-grid/integration-grid.component.html Outdated
[linkText]="integration.linkText"
[linkURL]="integration.linkURL"
[image]="integration.image"
[externalURL]="integration.type === 'sdk'"
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we store the 'sdk' and other strings relating to integration type into an enum just in case we change this value in the future to be SDK instead of sdk?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

👍 I like the suggestion, it also helps if other integration types are added too

361c820

@differsthecat
Copy link
Member

This is good on my end, @nick-livefront I did pull main and resolved the conflicts that were present in the messages.json file 🙂

cd-bitwarden
cd-bitwarden approved these changes Apr 18, 2024
View reviewed changes
Copy link
Contributor

@cd-bitwarden cd-bitwarden left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks great! Thank you :)

@nick-livefront
Copy link
Collaborator Author

This is good on my end, @nick-livefront I did pull main and resolved the conflicts that were present in the messages.json file 🙂

@differsthecat Thank you!

@differsthecat differsthecat removed the needs-qa Marks a PR as requiring QA approval label Apr 18, 2024
@differsthecat differsthecat merged commit 40ba15c into main Apr 18, 2024
62 of 63 checks passed
@differsthecat differsthecat deleted the sm/sm-956-integrations branch April 18, 2024 19:24
MGibson1 pushed a commit that referenced this pull request Apr 22, 2024
@nick-livefront @differsthecat @MGibson1
[SM-956] Secret Manager: Integrations Page (#8701)
35382f3 
* add navigation item for integrations and SDKs page

* Initial routing to Integrations & SDKs page

* Initial add of integrations component

* Initial add of SDKs component

* add secret manage integration images

* remove integration & sdk components in favor of a single component

* add integration & integration grid components

* add integrations & sdks

* rename page & components to integration after design discussion

* add external rel attribute for SDK links

* remove ts extension

* refactor: use pseudo element to cover as a link

* refactor: change secondaryText to linkText to align with usage

* update icon for integrations

* add new badge option for integration cards

* hardcode integration/sdk names

* add dark mode images for integrations and sdks

* update integration/sdk card with dark mode image when applicable

* refactor integration types to be an enum

* fix enum typings in integration grid test

---------

Co-authored-by: Robyn MacCallum <robyntmaccallum@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cd-bitwarden cd-bitwarden cd-bitwarden approved these changes

@differsthecat differsthecat Awaiting requested review from differsthecat

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@nick-livefront @differsthecat @cd-bitwarden