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

Jump to bottom

Adds possibility to define a goal description #10057

Merged
merged 5 commits into from Sep 25, 2016
Merged

Adds possibility to define a goal description #10057

merged 5 commits into from Sep 25, 2016

Conversation

sgiehl
Copy link
Member

@sgiehl sgiehl commented Apr 14, 2016

replaces #9469

image

image

fixes #7346

@sgiehl sgiehl added the Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. label Apr 14, 2016
@sgiehl sgiehl added this to the 3.0.0-b1 milestone Apr 14, 2016
@sgiehl sgiehl mentioned this pull request Apr 14, 2016
Closed
@sgiehl
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 Compare April 15, 2016 18:47
@tsteur
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
Copy link
Member Author

sgiehl commented Apr 18, 2016

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

@tsteur
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
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
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
@sgiehl sgiehl force-pushed the goal_description_3 branch 2 times, most recently from 7f3a57f to 47349b0 Compare September 1, 2016 20:48
@mattab
Copy link
Member

mattab commented Sep 19, 2016 •
edited

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 For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc. label Sep 19, 2016
@sgiehl
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

@sgiehl sgiehl removed the Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. label Sep 23, 2016
mattab
mattab reviewed Sep 25, 2016
View reviewed changes
Copy link
Member

@mattab mattab left a comment

Choose a reason for hiding this comment

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

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?

plugins/Goals/API.php
@@ -219,6 +225,11 @@ private function checkName($name)
return urldecode($name);
}

private function checkDescription($description)
{
return urldecode($description);
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

good point. It's not needed then!

@mattab mattab merged commit ab3eba1 into 3.x-dev Sep 25, 2016
@mattab mattab deleted the goal_description_3 branch September 25, 2016 22:22
@mattab
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

@mattab mattab mentioned this pull request Sep 25, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattab mattab mattab left review comments

Assignees

@sgiehl sgiehl

Labels
Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc.
Projects
None yet
Milestone
3.0.0-b1
Development

Successfully merging this pull request may close these issues.

Description/Comment on Goals
3 participants
@sgiehl @tsteur @mattab