-
Notifications
You must be signed in to change notification settings - Fork 33
refactor: 💡 remove ember-power-select duplicate dependency #3075
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
base: main
Are you sure you want to change the base?
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
|
||
| @use 'ember-basic-dropdown'; | ||
| @use 'ember-power-select'; | ||
|
|
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.
originally these imports got added when installing ember-basic-dropdown and ember-power-select.
ember-basic-dropdownis a peer dep for hds components. Though it looks like this@useisn't necessary at all.ember-power-selectwas installed since some styles were needed so that an invisible element doesn't incorrectly render in the dropdown. However, @hashicc noticed that this dep was being installed twice now (since its also installed for hds components. In order to avoid installing it twice a workaround is to import the styles inapp.js.
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.
I'm curious, did you need to add the <BasicDropdownWormhole /> that HDS mentions here?
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.
yup that was added here
hashicc
left a comment
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.
I know the docs offer the import 'ember-power-select/styles'; option but I'm not seeing the power select styles in the vendor.css or admin.css. I do see the power select classes from hds. I'm not sure if there are caveats to this method? I'm not sure if I'm missing something or maybe these styles aren't needed?
Looks like the styles are being injected directly into
EDIT |
The merge-base changed after approval.
0afe551 to
accf9fa
Compare
bc6257b to
546a4c3
Compare


Description
Remove duplicate dependency.
How to Test
Validate select dropdown still works here or locally.
Checklist
a11y-testslabel to run a11y audit tests if neededPCI review checklist
Examples of changes to security controls include using new access control methods, adding or removing logging pipelines, etc.