-
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: delete pki key functionality #18146
Conversation
hellobontempo
commented
Nov 29, 2022
•
edited
Loading
edited
constructor() { | ||
super(...arguments); | ||
this.formFields = expandAttributeMeta(this, ['keyId', 'keyName', 'keyType', 'keyBits']); | ||
get backend() { |
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.
Is it bad for this to be a getter? In other situations we've added it as an attribute in the adapter - but I felt it sometimes got lost there.
Happy to move it there instead!
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.
Ooh I love this! I wonder if we can standardize everywhere else 👀
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.
Another option is to use alias
on the service's currentPath
but I think it's potato potato
@@ -1,27 +0,0 @@ | |||
import { action } from '@ember/object'; |
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.
not deleted, just moved into the page/
folder
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! A couple small items to follow up on. Love to see it! 👏
|
||
@withFormFields(['keyId', 'keyName', 'keyType', 'keyBits']) |
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.
✨ 🤩
constructor() { | ||
super(...arguments); | ||
this.formFields = expandAttributeMeta(this, ['keyId', 'keyName', 'keyType', 'keyBits']); | ||
get backend() { |
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.
Ooh I love this! I wonder if we can standardize everywhere else 👀
constructor() { | ||
super(...arguments); | ||
this.formFields = expandAttributeMeta(this, ['keyId', 'keyName', 'keyType', 'keyBits']); | ||
get backend() { |
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.
Another option is to use alias
on the service's currentPath
but I think it's potato potato
} | ||
|
||
export default class PkiKeyDetails extends Component<Args> { | ||
@service declare router: { transitionTo: (route: string) => void }; |
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.
Would love to see the types here moved to either a typescript declare module, or imported directly from some stub types folder. Cloud-UI also has a pattern of doing @service declare readonly myService
but I'm not finding any luck with why. Couldn't hurt though!
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.
Okay! I wasn't sure how to do the module way. I had noticed the other way for this was:
import RouterService from '@ember/routing/router-service';
export default class PkiKeyDetails extends Component<Args> {
@service declare readonly router: RouterService;
...
what do you think?
}); | ||
}); | ||
|
||
test('it renders the page component', async function (assert) { |
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.
Should we add a test that confirms the delete action works?
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.
Beautiful! Thank you! 🙌
* add deletekey * fix types * move page components into folder * finish tests * make linting changes * declare flashmessages ts service * restructure pki test files * add delete test * add more folders