-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
feat(explore): Turn explore modes into tabs #89437
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
Conversation
This introduces a tab style UI for switching to/from aggregates mode. It's a bit of a hack to avoid changing the underlying data model as this is temporary as we figure out next steps.
Codecov ReportAll modified and coverable lines are covered by tests ✅ ✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## master #89437 +/- ##
==========================================
- Coverage 87.73% 82.96% -4.78%
==========================================
Files 10158 10161 +3
Lines 573544 573456 -88
Branches 22565 22554 -11
==========================================
- Hits 503188 475745 -27443
- Misses 69922 97112 +27190
- Partials 434 599 +165 |
…turn-explore-modes-into-tabs
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 👍 tested it out locally as well and I don't have anything to raise
<ToolbarVisualize equationSupport={extras?.includes('equations')} /> | ||
{mode === Mode.AGGREGATE && <ToolbarGroupBy />} | ||
{(extras?.includes('tabs') || mode === Mode.AGGREGATE) && <ToolbarGroupBy />} |
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 noticed this field does nothing in this PR until you switch to aggregate mode in the tabs. I assume this is because you mentioned wanting to put in a mini aggregate table below the chart, but wanted to call it out in case that wasn't the intention of this change.
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 was asked to make sure this change was behind a feature flag so we can test it prior to releasing it. This line line is so that if the tabs ui is not enabled, we fall back to the current behaviour of only showing the group by toolbar in aggregate mode.
This introduces a tab style UI for switching to/from aggregates mode. It's a bit of a hack to avoid changing the underlying data model as this is temporary as we figure out next steps.
This introduces a tab style UI for switching to/from aggregates mode. It's a bit of a hack to avoid changing the underlying data model as this is temporary as we figure out next steps.