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

[SM-660] move sm-no-items to CL #5059

Merged
merged 4 commits into from Apr 7, 2023
Merged

[SM-660] move sm-no-items to CL #5059

merged 4 commits into from Apr 7, 2023

Conversation

willmartian
Copy link
Contributor

Type of change

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

Objective

  • moves the sm-no-items component to the CL
  • renames to bit-no-items
  • adds Storybook stories

Code changes

  • bitwarden_license/bit-web/src/app/secrets-manager/*: Renamed all instances of sm-no-items; updated module imports

Screenshots

No visual changes.

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

@willmartian willmartian requested a review from a team March 22, 2023 22:15
@github-actions github-actions bot added the needs-qa Marks a PR as requiring QA approval label Mar 22, 2023
@willmartian willmartian requested a review from Hinton March 22, 2023 22:19
Copy link
Member

@Hinton Hinton 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, a few small notes. And should we add some quick notes what the component is used for in the component file, which should show up in storybook?

/**
 * Component for displaying a message when there are no items to display. Expects title, description and button slots.
 */

props: args,
template: `
<bit-no-items class="tw-text-main">
<ng-container slot="title">No items found</ng-container>
Copy link
Member

Choose a reason for hiding this comment

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

SecretsManagerSharedModule,
ServiceAccountsRoutingModule,
BreadcrumbsModule,
NoItemsModule,
Copy link
Member

Choose a reason for hiding this comment

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

Seems we didn't have to import this for any of the other secret modules, is this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The other components where it is used are declared in the shared SM module. I went ahead and just added NoItemsModule to the shared module's exports, so this is no longer needed.

libs/components/src/no-items/index.ts Outdated Show resolved Hide resolved
@willmartian
Copy link
Contributor Author

Looks good, a few small notes. And should we add some quick notes what the component is used for in the component file, which should show up in storybook?

/**
 * Component for displaying a message when there are no items to display. Expects title, description and button slots.
 */

Tried adding this in a few different places and couldn't get it to show up in Storybook. It looks like that feature might come out next release?

@willmartian willmartian requested a review from Hinton March 24, 2023 14:06
Hinton
Hinton previously approved these changes Mar 24, 2023
Copy link
Member

@Hinton Hinton left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@willmartian willmartian removed the needs-qa Marks a PR as requiring QA approval label Apr 7, 2023
@willmartian willmartian merged commit 36de1c8 into master Apr 7, 2023
57 of 58 checks passed
@willmartian willmartian deleted the sm/SM-660-no-items-cl branch April 7, 2023 15:05
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