fix(card): added missing sp-popover dependency#5449
Conversation
Rajdeepc
added
dependencies
🦋 Changeset detectedLatest commit: da1a070 The changes in this PR will be included in the next version bump. This PR includes changesets to release 84 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Branch previewReview the following VRT differencesWhen a visual regression test fails (or has previously failed while working on this branch), its results can be found in the following URLs:
If the changes are expected, update the |
Tachometer resultsChromecard permalinktest-basic
Firefoxcard permalinktest-basic
|
castastrophe
left a comment
castastrophe
left a comment
There was a problem hiding this comment.
A few questions about the update, thanks!
.changeset/cuddly-lands-work.md
Outdated
| '@spectrum-web-components/card': patch | ||
| --- | ||
|
|
||
| fix(card) - added missing sp-popover dependency in sp-card dependency tree |
There was a problem hiding this comment.
This is still reading like a commit message. Can we get in the habit of writing out changesets messages as detailed changelog notes?
There was a problem hiding this comment.
There was a problem hiding this comment.
Thanks @nikkimk for sharing the guidelines
package.json
Outdated
| "cem-plugin-module-file-extensions": "^0.0.5", | ||
| "chromatic": "^11.20.0", | ||
| "chromedriver": "^134.0.5", | ||
| "chromedriver": "^136.0.0", |
There was a problem hiding this comment.
This seems unrelated to the dependency work outlined in the description. Can we pull this out since its impact ends up being larger than the card component?
There was a problem hiding this comment.
Funny! I think @blunteshwar accidentally pushed it to my branch :P
| "@spectrum-web-components/checkbox": "1.6.0", | ||
| "@spectrum-web-components/divider": "1.6.0", | ||
| "@spectrum-web-components/icons-workflow": "1.6.0", | ||
| "@spectrum-web-components/popover": "1.6.0", |
There was a problem hiding this comment.
Does every card require a popover? It almost seems like this should be an optional dependency? When does a card use a popover?
There was a problem hiding this comment.
It should be a regular dependency, not an optional one. The component needs to be available when the toggles feature is used. As per my understanding optional dependencies are typically for features which can be completely replaced or deleted. We have never shipped any components with optionalDependencies
nikkimk
left a comment
nikkimk
left a comment
There was a problem hiding this comment.
There should be a ticket associated with this PR, either in GitHub or Jira. Also there were no testing instructions. Can you please add some more detail?
nikkimk
left a comment
nikkimk
left a comment
There was a problem hiding this comment.
Liquid gaffe torments mixologist (LGTM)
| '@spectrum-web-components/card': patch | ||
| --- | ||
|
|
||
| - **Fixed**: `sp-card` component relies on `sp-popover` for certain toggle interactive behaviors, but this dependency was missing from its dependency tree. |
castastrophe
force-pushed
the
rajdeep/missing-popover-dependency
branch
from
d51a467 to
c8d6038
Compare
caseyisonit
added
Status: Ready for merge
6 participants
Description
sp-cardhas a direct import ofimport '@spectrum-web-components/popover/sp-popover.js';as its direct dependency but it is missed as import in its dependency tree inpackage.json. Added missingsp-popoverdependency insp-card`Motivation and context
Related issue(s)
Screenshots (if appropriate)
Author's checklist
Reviewer's checklist
patch,minor, ormajorfeaturesManual review test cases
Verify the fix
Device review