Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Improve image graph axis display in email reports #20953

Merged
merged 6 commits into from
Jul 9, 2023
Merged

Improve image graph axis display in email reports #20953

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
809c6ea
Use unifont for French if available, remove NNBSP if unifont not avai…
bx80 Jun 30, 2023
57560fa
Updated UI test screenshots
bx80 Jun 30, 2023
9f333e1
Replace with NBSP instead of normal space, add FR to list of unifont …
bx80 Jun 30, 2023
3c3db7f
Do not use unicode font for French
bx80 Jul 6, 2023
817edc5
use higher values for image graphs
sgiehl Jul 7, 2023
27c1d79
updates expected UI test files
sgiehl Jul 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 19 additions & 2 deletions plugins/ImageGraph/StaticGraph.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,7 +248,12 @@ protected function initpData()
}
}

$this->pData->setAxisDisplay(0, AXIS_FORMAT_CUSTOM, '\\Piwik\\Plugins\\ImageGraph\\formatYAxis');
// Use a different formating method if not using unifont
$formatMethodName = 'formatYAxis';
if (strpos($this->font, 'unifont') === false) {
$formatMethodName = 'formatYAxisNonUnifont';
}
$this->pData->setAxisDisplay(0, AXIS_FORMAT_CUSTOM, '\\Piwik\\Plugins\\ImageGraph\\' . $formatMethodName);

$this->pData->addPoints($this->abscissaSeries, self::ABSCISSA_SERIE_NAME);
$this->pData->setAbscissa(self::ABSCISSA_SERIE_NAME);
Expand Down Expand Up @@ -376,7 +381,7 @@ private static function hex2rgb($hexColor)
}

/**
* Global format method
* Global format method - unifont
*
* required to format y axis values using CpChart internal format callbacks
* @param $value
Expand All @@ -386,3 +391,15 @@ function formatYAxis($value)
{
return NumberFormatter::getInstance()->format($value);
}

/**
* Global format method - non-unifont
*
* required to format y axis values using CpChart internal format callbacks
* @param $value
* @return mixed
*/
function formatYAxisNonUnifont($value)
{
return str_replace("\xE2\x80\xAF", " ", NumberFormatter::getInstance()->format($value));
bx80 marked this conversation as resolved.
Show resolved Hide resolved
}
12 changes: 6 additions & 6 deletions plugins/ImageGraph/StaticGraph/GridGraph.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,18 +22,18 @@ abstract class GridGraph extends StaticGraph

const DEFAULT_TICK_ALPHA = 20;
const DEFAULT_SERIE_WEIGHT = 0.5;
const LEFT_GRID_MARGIN = 4;
const LEFT_GRID_MARGIN = 20;
const BOTTOM_GRID_MARGIN = 10;
const TOP_GRID_MARGIN_HORIZONTAL_GRAPH = 1;
const RIGHT_GRID_MARGIN_HORIZONTAL_GRAPH = 4;
const TOP_GRID_MARGIN_HORIZONTAL_GRAPH = 10;
const RIGHT_GRID_MARGIN_HORIZONTAL_GRAPH = 20;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the changed screenshots it looks like the adjustments were actually only needed for the evolution chart.
The other charts now have additional unused space. Wouldn't it been have possible to only adjust the evolution one?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The other charts are also drawn very close to the edge of the image, for example the horizontal bar chart would likely clip the bar totals for numbers with four digits. I think adding few extra pixels of padding around them reduces the likelihood of other clipping issues and will look better if the images are embedded with other text.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we would actually need to automatically increase those margins if higher values are meant to be printed.
It might make sense to adjust the UI tests so they use higher values. I think we could achieve that without tracking additional numbers but simply modifying the results. I'll try something an maybe push some changes here to achieve that if easily possible.

const OUTER_TICK_WIDTH = 5;
const INNER_TICK_WIDTH = 0;
const LABEL_SPACE_VERTICAL_GRAPH = 7;

const HORIZONTAL_LEGEND_TOP_MARGIN = 5;
const HORIZONTAL_LEGEND_LEFT_MARGIN = 10;
const HORIZONTAL_LEGEND_BOTTOM_MARGIN = 10;
const VERTICAL_LEGEND_TOP_MARGIN = 8;
const VERTICAL_LEGEND_TOP_MARGIN = 10;
const VERTICAL_LEGEND_LEFT_MARGIN = 6;
const VERTICAL_LEGEND_MAX_WIDTH_PCT = 0.70;
const LEGEND_LINE_BULLET_WIDTH = 14;
Expand Down Expand Up @@ -358,7 +358,7 @@ protected function getGridTopMargin($horizontalGraph, $verticalLegend)
if ($horizontalGraph) {
$topMargin = $ordinateMaxHeight + self::TOP_GRID_MARGIN_HORIZONTAL_GRAPH + self::OUTER_TICK_WIDTH;
} else {
$topMargin = $ordinateMaxHeight / 2;
$topMargin = $ordinateMaxHeight / 2 + self::TOP_GRID_MARGIN_HORIZONTAL_GRAPH;
}

if ($this->showLegend && !$verticalLegend) {
Expand Down Expand Up @@ -398,7 +398,7 @@ protected function getGridRightMargin($horizontalGraph)
list($ordinateMaxWidth, $ordinateMaxHeight) = $this->getMaximumTextWidthHeight($this->ordinateSeries);
return self::RIGHT_GRID_MARGIN_HORIZONTAL_GRAPH + $ordinateMaxWidth;
} else {
return 0;
return self::RIGHT_GRID_MARGIN_HORIZONTAL_GRAPH;
}
}

Expand Down
4 changes: 2 additions & 2 deletions plugins/ImageGraph/tests/UI/expected-screenshots/ImageGraph_evolution_graph.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
4 changes: 2 additions & 2 deletions plugins/ImageGraph/tests/UI/expected-screenshots/ImageGraph_horizontal_bar.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
4 changes: 2 additions & 2 deletions plugins/ImageGraph/tests/UI/expected-screenshots/ImageGraph_vertical_bar.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.