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.

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

Adds possibility to define a goal description #10057

Merged
merged 5 commits into from Sep 25, 2016

Conversation

Projects
None yet
3 participants
@sgiehl
Copy link
Member

sgiehl commented Apr 14, 2016

replaces #9469

image

image

fixes #7346

@sgiehl

This comment has been minimized.

Copy link
Member Author

sgiehl commented Apr 14, 2016

@tsteur In the old PR I had changed the goal page to show the description as kind of subheadline under the goal title or as tooltip. See:

image

image

Is there any simple possibility to do that with the new container/widget framework?

@sgiehl sgiehl force-pushed the goal_description_3 branch 2 times, most recently from bf7500c to bd13f3b Apr 14, 2016

@tsteur

This comment has been minimized.

Copy link
Member

tsteur commented Apr 18, 2016

Not super easily unless it was a feature of the HtmlTable or EvolutionGraph visualization but I presume it should appear with any visualization?

If it should work with any visualization there are multiple ways to solve it. We could eg enrich WidgetConfig if it should be a feature of all widgets https://github.com/piwik/piwik/blob/3.x-dev/core/Widget/WidgetConfig.php or we could add a $description property to https://github.com/piwik/piwik/blob/3.x-dev/core/Report/ReportWidgetConfig.php which would be only for Report widgets. I think ReportWidgetConfig would not be right though and it should maybe rather add a new description property to API.getReportMetadata via the Report class https://github.com/piwik/piwik/blob/3.x-dev/core/Plugin/Report.php#L516 . In JS we already merge WidgetMetadata and ReportMetadata to get the report documentation see https://github.com/piwik/piwik/blob/3.x-dev/plugins/CoreHome/angularjs/widget/widget.directive.js#L61-L67 . Here we could then also merge a widget.description = report.description and display it conditionally below the headline (if present) https://github.com/piwik/piwik/blob/3.x-dev/plugins/CoreHome/angularjs/widget/widget.directive.html#L12 . It will then work for all reports. Is this kind of understandable?

  • Add a description to reportMetadata
  • Merge description in widget.directive.js
  • Show it in widget.directive.html

Maybe there would be a better name than description? Not sure if it is confusing to have documentation vs description.

@sgiehl

This comment has been minimized.

Copy link
Member Author

sgiehl commented Apr 18, 2016

Thx for the explanation. Need to have a closer look at that tomorrow.

@tsteur

This comment has been minimized.

Copy link
Member

tsteur commented Apr 22, 2016

I would recommend to not work on this for now. Looks like we will move the rendering of the headline back into the widget etc. Will keep you updated.

@tsteur

This comment has been minimized.

Copy link
Member

tsteur commented Aug 30, 2016

The headline for reports and everything is now rendered in plugins/CoreHome/templates/_dataTable.twig. To make it possible to show the description we would possibly need to add a new ViewDataTable config property in core/ViewDataTable/Config.php and then check if a value is set in the twig template and if so, print it :) That should work, let me know if not

@sgiehl sgiehl force-pushed the goal_description_3 branch from bd13f3b to d286375 Aug 30, 2016

@sgiehl

This comment has been minimized.

Copy link
Member Author

sgiehl commented Aug 30, 2016

I've rebased the branch and updated the new angular javascript. Will have a look at the rendering of the description the next days...

@sgiehl sgiehl self-assigned this Aug 30, 2016

@tsteur tsteur force-pushed the 3.x-dev branch from 954175e to 5d52b77 Sep 1, 2016

@sgiehl sgiehl force-pushed the goal_description_3 branch 2 times, most recently from 7f3a57f to 47349b0 Sep 1, 2016

@mattab

This comment has been minimized.

Copy link
Member

mattab commented Sep 19, 2016

LGTM. Minor feedback:

  • Maybe the input field could be wider (but still on one line) as description will be longer than the name, when set.
  • an integration test is failing test_getGoal_shouldReturnExistingGoal
  • Left couple comments (next time I'll try the new github Review feature)

@mattab mattab added the Enhancement label Sep 19, 2016

@sgiehl sgiehl force-pushed the goal_description_3 branch from 47349b0 to ba30190 Sep 23, 2016

@sgiehl sgiehl force-pushed the goal_description_3 branch from ba30190 to 4d5e443 Sep 23, 2016

sgiehl added some commits Sep 23, 2016

@sgiehl

This comment has been minimized.

Copy link
Member Author

sgiehl commented Sep 23, 2016

I've updated the code, so the description will now be shown in the reports. failing tests should also be fixed

@mattab
Copy link
Member

mattab left a comment

The text is displayed in Black below the Goal name, whereas it was in grey in your original screenshot. was it intentional to show it in black?

@@ -219,6 +225,11 @@ private function checkName($name)
return urldecode($name);
}
private function checkDescription($description)
{
return urldecode($description);

This comment has been minimized.

Copy link
@mattab

mattab Sep 25, 2016

Member

as the description field is VARCHAR 255 maybe we should check here that the description is maximum 255 chars?

This comment has been minimized.

Copy link
@sgiehl

sgiehl Sep 26, 2016

Author Member

name is varchar 50 and not checked re length as well. Shall we add such checks?

This comment has been minimized.

Copy link
@mattab

mattab Sep 26, 2016

Member

good point. It's not needed then!

@mattab mattab merged commit ab3eba1 into 3.x-dev Sep 25, 2016

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/travis-ci/push The Travis CI build failed
Details

@mattab mattab deleted the goal_description_3 branch Sep 25, 2016

@mattab

This comment has been minimized.

Copy link
Member

mattab commented Sep 25, 2016

Cheers @sgiehl - I'm merging already but feel free to make minor changes post-merge if needed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.