-
Notifications
You must be signed in to change notification settings - Fork 0
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
[DSS-462] add Popover #100
Conversation
✅ Deploy Preview for pine-design-system ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few styling and story-related comments, but overall this is looking great!
libs/core/src/components/pds-popover/stories/pds-popover.stories.js
Outdated
Show resolved
Hide resolved
libs/core/src/components/pds-popover/stories/pds-popover.docs.mdx
Outdated
Show resolved
Hide resolved
❌ Deploy Preview for pine-icons failed.
|
4bda89a
to
cc05124
Compare
…ltip to pds-popover
…for overlay action list content
Co-authored-by: Monica Wheeler <monica.wheeler@kajabi.com>
Co-authored-by: Monica Wheeler <monica.wheeler@kajabi.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙌
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM! One tiny nitpick around formatting and no e2e tests (that may be intentional?). Just let me know and I'll give the 👍🏼 if that's the case.
|
||
expect(element?.querySelector('.pds-popover')).toHaveClass('pds-popover--is-open'); | ||
}); | ||
it('should toggle the popover', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing empty line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One last non-blocking comment, but overall this is great work! LGTM! 👍🏼
<pds-button variant="secondary">Help</pds-button> | ||
</pds-popover>`; | ||
|
||
const AvatarDropdownTemplate = (args) => html` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe as a follow up at some point we could change this story to something like Trigger
or Trigger Types
? I found Avatar Popover in the sidebar confusing initially and thought it was some type of variant. Not a major blocker, just felt it's worth a mention.
5f89487
to
46929a2
Compare
Closing in favor of #134 |
Description
Fixes #DSS-462
Type of change
How Has This Been Tested?
Please describe the tests you've added and run to verify your changes.
Provide instructions so that we can reproduce.
Please also list any relevant details for your test configuration.
Test Configuration:
Checklist:
If not applicable, leave options unchecked.