-
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
Deangularize Dashboard, but with Hooks #84913
Deangularize Dashboard, but with Hooks #84913
Conversation
💔 Build Failed
Failed CI Steps
Test FailuresChrome UI Functional Tests.test/functional/apps/dashboard/empty_dashboard·js.dashboard app using current data empty dashboard "before all" hook for "should display empty widget"Standard Out
Stack Trace
Chrome UI Functional Tests.test/functional/apps/dashboard/empty_dashboard·js.dashboard app using current data empty dashboard "after all" hook for "should add new visualization from dashboard"Standard Out
Stack Trace
Chrome UI Functional Tests.test/functional/apps/visualize/_data_table_notimeindex_filters·ts.visualize app data table with index without time filter filters should add to dashboard and allow filteringStandard Out
Stack Trace
and 12 more failures, only showing the first 3. Metrics [docs]Module Count
Async chunks
Distributable file count
Page load bundle
To update your PR or re-run it, just comment with: |
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.
In testing this out, most of the test cases work great, and the custom hook code looks really clean.
The most problematic use case (saving a dashboard as new) is still throwing react errors. Not the same error as I had before, but two different ones:
The node you're attempting to unmount was rendered by React and is not a top-level container.
and
Warning: render(...): Replacing React-rendered children with a new root component.
That said, these two warnings are more easily fixed than the one I was running into earlier, so I will attempt to merge the custom hooks from here into the original PR.
Thanks again for looking into this!
…eRenderer to render dashboard embeddable
Summary
This is a Hooks-based approach to #82909 ... I had to merge the previous branch into a single commit to make the changes... hopefully this makes sense.