Add horizontal bar chart

[rafawendel]

What type of PR is this?

• Feature

Description

Addition of the horizontal bar chart as a display option for the bar chart

Plotly currently has the support for horizontal bar charts via the addition of the orientation property to the chart object.

In order to do so, I added a UI checkbox that will pass this option down and added a check for it that will add the Plotly property to the chart.

Problem 1: Plotly doesn't invert the axes, which is the behavior expected by the users

Solution 1: Remapped values so as to exchange x and y

Problem 2: Remapping the values gives UI responsiveness, but it leads to UI misbehavior, such as unexpected axes (x will not accept multiple, y won't allow sorting), nonworking normalized to percent and stacked charts

Solution 2: In order to have X behaving as Y and vice-versa, it sounded best to me to just invert the axes after all configuration was done, which lead to the expected behavior.

Remaining problems:

• Working axes inversion
• Working stacked charts
• Add tests
• Working normalized percentages (disabled for horizontal charts at the moment)
• Add UI responsiveness to the newer version, as the X/Y setting options will set the opposite axes
• Pass current X scale to Y and vice versa, as Y won't auto-detect category, which will lead to an empty chart

[rafawendel]

I was insecure about how deep in the code I should get my hands on, so I tried to keep my changes at a minimum. Having the rest of the features working would involve changing ColumnMappingSelect and X/Y axes settings. I think it's better to hear your thoughts on it first.

[rafawendel]

Yesterday I realized when you mixed the horizontal chart with other charts in the "series" menu it would result in a mix of axes, as I have added the inversion only in the bar chart plotly preparing function.

In order to fix that, I changed the inversion to the bottom of the prepareSeries function, which results in a better behavior:

In the above example you can notice stacking is working as usual. Given the fact the inversion should work for all other 2d charts, I simply added line and area to the list of options. However, doing so results in misbehavior for stacked charts.

Also it requires further setup to work properly. Due to that I decided to remove them and keep it only for bar charts.

I was thinking whether the best option is to keep this as a checkbox or I should add it to the dropdown chart options.

Either way, I realized that in order to proceed the following is needed

• Minor refactor of ColumnMappingSelect and MappingTypes
• Minor refactor of YAxisSettings
• New abstraction for setting axes labels and exchanging scales via options

The two of them are tailored for only working with the regular charts and would need changes to adapt to a more flexible UI this one requires.

[gabrieldutra]

Hi @rafawendel, thanks for all the detailed explanation on the context here 🙂. I've checked the feature + code and I would like to suggest a bit of a different approach.

You've probably noticed already that we have lots of customizations over Plotly and they take into account the "x" and "y" values. Some of those customizations are indeed on the "prepare" phase (those that are just some Redash -> Plotly option mapping), but others are "updates". Considering in this case we actually invert both axes, we will need to include this as an "update" in the exactly right spot, i.e.: inverting the axes and their settings as the Last step (or almost the Last 😝) should make it work as we want and with minimal risk.

(@kravets-levko you're invited to correct my understanding or to suggest other approaches 🙂)

I suggest you to check this file and its uses, so that you can base to create a new updater. There is a comment explaining the updaters in

redash/viz-lib/src/visualizations/chart/Renderer/initChart.js

Lines 16 to 22 in a1255b4

	// This utility is intended to reduce amount of plot updates when multiple Plotly.relayout
	// calls needed in order to compute/update the plot.
	// `.append()` method takes an array of two element: first one is a object with updates for layout,
	// and second is an optional function that will be called when plot is updated. That function may
	// return an array with same structure if further updates needed.
	// `.process()` merges all updates into a single object and calls `Plotly.relayout()`. After that
	// it calls all callbacks, collects their return values and does another loop if needed.

I've quickly tested this by adding an updater based on the updateYRanges with an extra plotlyData arg:

if (options.invertedAxes) {
  each(plotlyData, series => {
    series.orientation = "h";
    const { x, y } = series;
    series.x = y;
    series.y = x;
  });

  updates.xaxis = layout.yaxis;
  updates.yaxis = layout.xaxis;

  console.log(layout);
  console.log(plotlyData);
}

Result:

Note that the other features will keep working and the settings will also change accordingly. Extra: we can later check if it is worth it to rename the UI items too 😁 ("X axis" -> "Y axis" and so on).

Thanks again and LMK if I can be of any more help.

[rafawendel]

I've finished a working version of the horizontal bar chart. The final behavior is the same as Gabriel's GIF.

In order to have the labels inverted in the UI I had to do some sneaky stuff which should be made into abstractions for better long-term maintenance.

Also I couldn't fully grasp how the update functions work. In the example Gabriel gave adding layout properties to the update object resulted in changes to the layout, which was not the case for my function.

Finally, there is still one problem: charts with double y axes will yield weird behaviors. To fix that I would have to go I little deeper into how yaxis2 is passed down to Plotly and have a xaxis2 instead. That would be interesting, as it's listed as an example at the documentation.

[rafawendel]

I've noticed I had forgotten to reset the state after the user changed charts, which would result in inversion of axes in other charts. Also, horizontal inversion can be safely applied to line charts. It should work for area charts, but the baseline for the area gets messed.

[rafawendel]

Generalizing this pattern to all string arguments might be an abstraction worth considering

[arikfr]

This is great work, @rafawendel! Beyond the code, it was really helpful following through the thought process in the PR description and the comments.

In terms of code, looking good - there are two small nits I noticed while going through it (see comments below).

In terms of functionality: currently the chart stops rendering when moving one of the series to the second Y axis. You mentioned already that there are issues with this, but my suggestion is: let's disable this functionality when Horizontal Chart is selected. If there will be demand/valid use cases, we can look into adding support for it. Wdyt?

[arikfr]

Something in the formatting looks off, worth running Prettier here.

[gabrieldutra]

npm run prettier currently doesn't include the viz-lib 😅, I'll fix that.

---

@rafawendel I think for it to work in vscode you'll need to manually specify the settings path to client/prettier.config.js for the workspace.

[arikfr]

This is no longer needed?

[arikfr]

Suggested change

	Horizontal chart
	Horizontal Chart

[rafawendel]

This is great work, @rafawendel! Beyond the code, it was really helpful following through the thought process in the PR description and the comments.

In terms of code, looking good - there are two small nits I noticed while going through it (see comments below).

In terms of functionality: currently the chart stops rendering when moving one of the series to the second Y axis. You mentioned already that there are issues with this, but my suggestion is: let's disable this functionality when Horizontal Chart is selected. If there will be demand/valid use cases, we can look into adding support for it. Wdyt?

Thank you @arikfr, it's flattering to hear it coming from you!

I find it a great idea! I was wondering whether it would be as laborious to add the second x axis as just disabling the second y axis. Now the second one sounds best.

Look forward to doing it ASAP

[rafawendel]

TLDR Code is ready: it now deals with right Y axes. Future refactoring is needed.

@gabrieldutra and @arikfr I am done with dealing with the right Y axis. When the horizontal axis is disabled the right Y axis will disappear from the series' tab, Y Axis (which will be displayed as X Axis) and it's state will be reset to left Y (comments below).

Therefore I think I've finished all details needed for merger. Surely I will work on any further issues you might point out or future improvements/corrections.

Conclusion

Working in this involved getting my hands dirty, as I had to modify several functions around the code. The greatest problem is that most of these changes are hardcoded, which can lead to poor maintenance.

In order to improve this, we would need to further discuss on the priority of creating abstractions that would increase the robustness of this feature.

[rafawendel]

By doing this, every time horizontal chart changes state, all series go back to left Y axis.

[kravets-levko]

Just a few last things I'd like to see in this PR, and it's good to go 👍

[rafawendel]

Guess it's done! Replaced the array functions with lodash's and added proptyes, which should avoid any problems

[kravets-levko]

🌟

[rafawendel]

Funny thing is that I had thought of doing this. Hope to get better at it with time

[kravets-levko]

Funny thing is that I had thought of doing this. Hope to get better at it with time

Don't worry too much, you're already very good 👍 Everything else will come with experience (just learn all the time) 😉

[gabrieldutra]

Congrats on your first merged PR @rafawendel! 🎉

[gaecoli]

this can be order by values?