-
Notifications
You must be signed in to change notification settings - Fork 15.3k
feat(stack by dimension): add a stack by dimension dropdown list #32707
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
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.
Review by Korbit AI
Korbit automatically attempts to detect when you fix issues in new commits.
Category | Issue | Fix Detected |
---|---|---|
Missing conditional visibility for stack dimension control ▹ view | ✅ | |
Redundant shouldMapStateToProps Function ▹ view | ✅ | |
Incorrect Operator Precedence in Dimension Index Calculation ▹ view | ✅ |
Files scanned
File Path | Reviewed |
---|---|
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/constants.ts | ✅ |
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/types.ts | ✅ |
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Bar/controlPanel.tsx | ✅ |
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Need a new review? Comment
/korbit-review
on this PR and I'll review your latest changes.Korbit Guide: Usage and Customization
Interacting with Korbit
- You can manually ask Korbit to review your PR using the
/korbit-review
command in a comment at the root of your PR.- You can ask Korbit to generate a new PR description using the
/korbit-generate-pr-description
command in any comment on your PR.- Too many Korbit comments? I can resolve all my comment threads if you use the
/korbit-resolve
command in any comment on your PR.- On any given comment that Korbit raises on your pull request, you can have a discussion with Korbit by replying to the comment.
- Help train Korbit to improve your reviews by giving a 👍 or 👎 on the comments Korbit posts.
Customizing Korbit
- Check out our docs on how you can make Korbit work best for you and your team.
- Customize Korbit for your organization through the Korbit Console.
Feedback and Support
const idxSelectedDimension = formData.metrics.length > 1 ? 1 : 0 | ||
+ chartProps.rawFormData.groupby.indexOf(stackdimension); |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Bar/controlPanel.tsx
Outdated
Show resolved
Hide resolved
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Bar/controlPanel.tsx
Outdated
Show resolved
Hide resolved
@jpchev please add a comprehensive description + screens to this PR so reviewers can first validate the design, and then look at the implementation details. I know these may be covered by the linked GH Issue, but the discussion is pretty lengthy. I think the feature is great, but I feel we need to be very careful of how this is implemented to ensure we don't pile on technical/design debt that will be difficult to clean up later. |
This is looking cool! Do you need to add the "Stack by" dimension to the "Dimensions" input? If so, this works fine as-is. If you don't, I'd be still be happy with: ... but if that entails a lot of work (which it might) then I still like this improvement :) |
@villebro I added a description for this PR |
How is this going to interact with the Stack control that already exists? |
I'm a little bit confused about this since not everything is stacked according to your video. Is this more of a group by dimension or something? |
@jpchev I reviewed the video and in general I think this is an elegant and backwards compatible solution to the problem 👍 I will review in more detail in the coming days. |
as far as I understand, the new feature has no interaction with the existing stack control |
yes, this is a sort of 'group by', but it's called 'stack' in the Apache Echarts vocabulary (see https://echarts.apache.org/handbook/en/how-to/chart-types/bar/stacked-bar/). |
Wouldn't be better to offer a control to define how things are stacked AFTER you set |
I don't know, what is the existing stack doing? |
It's stacking the bars under a common name. That's why all bars appear in the same stacking group. It seems you're trying to offer a way to modify this logic and that's why I think they are related. |
yes, they are probably related and I think that the existing stack feature should become a particular case of a broader feature which should let user stack by a label in each series. This label would work as a group by criteria, as it is implemented in Apache Echarts |
Hi, @jpchev. I'm trying to implement the stacked chart as you did above for the below dataset.
Can you help me understand by giving a demo for this, I'm not able to build the chart for the dataset as you did. |
hello,
|
@kgabryje @michael-s-molina @rusackas I've removed the checkbox and bound the visibility of the "dimension to stack by" dropdown to the value "Stack" of the existing "Stacked style" control. Can you please have a look? |
@michael-s-molina Processing your ephemeral environment request here. Action: up. More information on how to use or configure ephemeral environments |
@michael-s-molina Ephemeral environment spinning up at http://35.167.64.30:8080. Credentials are 'admin'/'admin'. Please allow several minutes for bootstrapping and startup. |
Added a test chart here. Controls look a lot cleaner with the conditional rendering now, thank you! Seems to work well in both vertical and horizontal mode. I still feel the dimension added to "Stack by" feels more like a "Split by" or "Create Stacks for" but again, maybe it's just me expecting "Stack by" to be the dimension that IS stacked. I think the tooltip does help explain the situation, though. |
Good, thanks for reviewing, can we consider having the feature merged? I can change the labels if that's better (replace "Stack by") |
@rusackas I replaced "Dimension to stack by" with "Dimension to split by", is that better? |
@rusackas @michael-s-molina @villebro any update on this feature? |
is this feature considered as abandoned or not relevant anymore? @rusackas @michael-s-molina @villebro |
Thanks for the patience @jpchev but as you can imagine, we receive many review requests. I pulled your branch and it looks good.
I can approve and merge the PR with these changes. |
@michael-s-molina I've restored the package-lock.json without my changes thank you |
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.
Thank you for the improvement, for addressing the comments, and for the patience @jpchev!
thank you, I'm very happy and it creates huge value for me |
…che#32707) Co-authored-by: CAPELLI Giampaolo <[email protected]>
…che#32707) Co-authored-by: CAPELLI Giampaolo <[email protected]> (cherry picked from commit 5b2f1bb)
SUMMARY
feature for bar chart, to stack by dimension, see
#32496
basically, this is a new option to let users select a dimension to 'stack by', see the video here
stackByDimension.mp4
the dimension to 'stack by' can be selected between the dimensions defined by the user, in the Customize tab
fixes: #32496