-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
UI: Hds::Dropdown replace PopupMenu #25321
UI: Hds::Dropdown replace PopupMenu #25321
Conversation
ui/app/lib/attach-capabilities.js
Outdated
@@ -34,7 +34,7 @@ import { isArray } from '@ember/array'; | |||
export default function attachCapabilities(modelClass, capabilities) { | |||
const capabilityKeys = Object.keys(capabilities); | |||
const newRelationships = capabilityKeys.reduce((ret, key) => { | |||
ret[key] = belongsTo('capabilities', { async: false, inverse: null }); | |||
ret[key] = belongsTo('capabilities', { async: true, inverse: null }); |
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.
just out of curiosity why was it changed to async: true
?
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.
Having it false was causing an error, I can't tell you much more than that!
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.
Specifically it caused this test to fail: auth-config-form options: it submits data correctly
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.
Just looked more into this and I think by updating the test we can leave this as async: false
. I'll test the options locally as well to make sure they submit properly
This reverts commit 49eca9c.
Co-authored-by: Kianna <[email protected]>
@hasChevron={{false}} | ||
data-test-popup-menu-trigger | ||
/> | ||
<dd.CopyItem @text={{meta.public_key}} @copyItemTitle="Copy Public Key" /> |
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.
do we still want a flash message 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.
No, the copyItem component handles a successs icon
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.
great work Claire and Chelsea! thank you for glimmerizing some of the components! ✨🚀
This PR updates most of the instances of PopupMenu to the HDS Dropdown component.
The PopupMenu in
access/methods.hbs
remains because removing it caused seemingly unrelated errors in tests. We will address that separately in its own PR, as it might require other refactors unrelated to simply replacing PopupMenu.