-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[Spaces WIP] Additional space management UI #18607
Conversation
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.
This is awesome! Mostly just questions at this point. Do you envision us wanting to move to Redux with these screens or just keep them straight React?
src/dev/jest/config.js
Outdated
@@ -9,6 +9,7 @@ export default { | |||
'<rootDir>/src/cli_plugin', | |||
'<rootDir>/src/dev', | |||
'<rootDir>/packages', | |||
'<rootDir>/x-pack/plugins' |
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 don't think we want this here, as all the x-pack tests are run inside of x-pack itself and this would make the OSS tests automatically pick up the x-pack jest tests.
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.
Oops, good catch! I'll remove this.
); | ||
}); | ||
|
||
$q.all(deleteOperations) |
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 use $q
even in React? I
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 keep forgetting that Promise.all
exists on native Promises...I'll change to use that. I brought $q
in to prevent pulling in another dependency, but it's a non-issue. I'll fix this too.
* you may not use this file except in compliance with the Elastic License. | ||
*/ | ||
|
||
export class SpacesDataStore { |
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.
Are we using a similar construct on the other management pages?
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 think this is the first React management screen -- from what I can tell, the Angular management screens to this client-side as well, but they are using angular's filter
mechanism to do most of that work, which isn't available to us.
@kobelb We may want Redux at some point here. I didn't want to bring it in until the screens were complex enough to warrant the framework, and until I was half-way confident in the structure. |
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.
LGTM
No description provided.