-
Notifications
You must be signed in to change notification settings - Fork 8.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
[Graph] Listing page and folder restructuring #44068
Conversation
💔 Build Failed |
💔 Build Failed |
💔 Build Failed |
💔 Build Failed |
💚 Build Succeeded |
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.
👍 Design looks good, thanks for making this consistent with other app home pages. The CSS changes were just file renames and one path change.
retest |
💔 Build Failed |
retest |
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.
Code LGTM, wise to 'quarantine' angular stuff. tested in Safari, Chrome, Firefox.
2 problems I encountered:
- Deleting a graph record doesn't work
- When navigating back via breadcrumb I always get the 'Discard changes to workspace?' modal. Even when I saved just before, or didn't change anything
💔 Build Failed |
Hey @kertal, thanks for your review.
That was intentional, currently the message is only shown when navigating away from the current workspace via the
Good point, recent master merge broke that. |
💚 Build Succeeded |
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.
Code LGTM, deleting items from graph workspace works now. Think it would be nice to have a functional test for deleting, however since it's all part of a big refactoring, there's no priority here,
@kertal Thanks, it's on the list as the very next item after the thing I'm currently working on |
@@ -63,6 +63,7 @@ export interface ChromeBreadcrumb { | |||
text: string; | |||
href?: string; | |||
'data-test-subj'?: string; | |||
onClick?: () => 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.
onClick?: (event: MouseEvent) => void
?
it's required for "open in new tab"?
@restrry Good catch, I looked into the EUI code and the property is passed to |
💚 Build Succeeded |
Summary
This PR adds a listing page to Graph to make it consistent with other apps like Maps, Visualize or Dashboards. It uses the
table_list_view
component that is also used for Dashboard and Visualize.Within the workspace view, the
Open
andDelete
menu actions are gone (because those are handled on the listing page)This PR also separates the legacy angular components into a separate folder
angular
, to move them over into their respective folders once they are converted over to typescript and react.Platform team: I added the
onClick
property to the types of breadcrumbs, this is supported by the underlying EUI component but wasn't exposed through the core types yet.Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.For maintainers