Make segment title clickable and add segment definition to tooltip. #15269
Conversation
|
Are there any important fixes in there? Otherwise rather move it to 4.0 maybe. |
|
No important fixes, just seemed small enough for 3.13.1. Will move it. |
| @@ -4,8 +4,8 @@ <h3>{{ 'General_Comparisons'|translate }}</h3> | |||
| <div class="comparison card" ng-repeat="comparison in $ctrl.comparisonsService.getSegmentComparisons() track by $index"> | |||
| <div class="comparison-type">{{ 'General_Segment'|translate }}</div> | |||
|
|
|||
| <div class="title" title="{{ comparison.title }}"> | |||
| {{ comparison.title }} | |||
| <div class="title" title="{{ comparison.title }}<br/>{{ comparison.params.segment }}"> | |||
There was a problem hiding this comment.
not sure, but does it maybe need some escaping for XSS, as a segment definition can contain any string?
There was a problem hiding this comment.
angularjs does this by default, so should be ok. (Did a quick test and couldn't get anything to happen.)
There was a problem hiding this comment.
@diosmosis this is what it looks for me
with this segment
in the DB the segment is stored as pageUrl=@b
|
@diosmosis any update on this? |
|
@diosmosis be good to look into this when you can |
|
@diosmosis seems some tests are failing now because the font changes slightly? https://builds-artifacts.matomo.org/matomo-org/matomo/4.x-dev/39205/Comparison_visitors_overview.png Wondering if it is because there's now no newline anymore and the browser renders it slightly differently? https://github.com/matomo-org/matomo/pull/15269/files#diff-35a447f52ea628304209b9dcf28812baR8 I suppose be fine to add this newline again? |
|
@tsteur could re add it and check, or could just copy over the screenshots? I don't really have an opinion. |
|
Not really. I'll test and add the spaces. Maybe it fixes the screenshots then it would avoid conflicts with other PR that updates same screenshots. |
|
@diosmosis didn't seem to change anything. Seems to be maybe because of adding a title? |
|
probably, maybe the rendering changes order or something and there's some tiny subtle change. can just update the screenshots? |
|
sounds good 👍 |
mattab
added
the
c: Usability
Couple small changes after talking to @mattab