Bar graph should only have Users and Unique Visitors options when they are valid #14332
Conversation
|
|
||
| protected function generateSelectableColumns() | ||
| { | ||
| $selectableColumns = array('nb_visits', 'nb_actions'); |
There was a problem hiding this comment.
@katebutler not sure about the columns we add by default. Many reports won't have the columns nb_actions, nb_conversions, revenue, nb_uniq_visitors and nb_users. Some reports might not even have the nb_visits. Did you copy this logic from somewhere?
Otherwise we could maybe simply set the first non-label column from the first row?
And if there is only label in the first row, we could maybe default to nb_visits?
There was a problem hiding this comment.
The existing logic was to just use all of them and hope for the best 😄 but I have reworked it to use all columns except label if none of these are present in the dataset
|
|
||
| protected function ensureValidColumnsToDisplay() | ||
| { | ||
| $columnsToDisplay = $this->config->columns_to_display; |
There was a problem hiding this comment.
I wonder if this logic here is needed? I wonder about possible side effects where reports declare columns_to_display but not selectable_columns.
In general this is a great addition that makes sense though. If something is not selectable, we shouldn't show it. Just scared about possible side effects or breaking something. Might not be an issue though but have a feeling there are some reports that might break as I can't find where the reports usually add this info.
There was a problem hiding this comment.
We can quickly chat about it tmrw might be easiest
There was a problem hiding this comment.
I've changed it to match against $dataTable->getColumns() rather than selectable_columns - this should hopefully avoid any side effects.
These should only be displayed when the visualization's date range supports these metrics (i.e. for a single date, but not a date range). This prevents the user from making selections which will trigger JS errors and prevent the visualization from rendering anyway.
Fixes #14042