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

Improve no data tracked yet screen #17367

Merged
merged 20 commits into from Mar 29, 2021
Merged

Improve no data tracked yet screen #17367

merged 20 commits into from Mar 29, 2021

Conversation

flamisz
Copy link
Contributor

@flamisz flamisz commented Mar 22, 2021

Description:

fixes #16787

Review

  • Functional review done
  • Potential edge cases thought about (behavior of the code with strange input, with strange internal state or possible interactions with other Matomo subsystems)
  • Usability review done (is anything maybe unclear or think about anything that would cause people to reach out to support)
  • Security review done see checklist
  • Code review done
  • Tests were added if useful/possible
  • Reviewed for breaking changes
  • Developer changelog updated if needed
  • Documentation added if needed
  • Existing documentation updated if needed

@flamisz flamisz self-assigned this Mar 22, 2021
@flamisz flamisz mentioned this pull request Mar 22, 2021
Merged
@flamisz
Copy link
Contributor Author

flamisz commented Mar 22, 2021 •
edited

Couple of example websites I used trying different CMSs:

type site
matomo.org - WP http://matomo.org
squarespace https://est-bostoncopywriter.com
wix https://www.thegrilledcheesefactory.fr
joomla https://www.nintendo.se
shopify + GTM https://www.manitobah.com
sharepoint https://incometaxindia.gov.in

@flamisz
Copy link
Contributor Author

flamisz commented Mar 22, 2021

Hi @tsteur,
the current page checks the WhiteLabel plugin and hides URLs (not all tho) to matomo.org. How strictly we'd like to follow that in the new solution? All the links that go to matomo.org? That way we can't show any instructions/guides from the FAQ. And what about links from the development documentation?

These are the links I use now:

  • CMS integration guides
  • GTM guide
  • the /integrate page from matomo.org
  • tracking dev documentation
  • tag manager guide
  • and guides for the other pages (log analytics, SDKs, HTTP tracking guide)

Thanks, Zoltan

@tsteur
Copy link
Member

tsteur commented Mar 22, 2021

The links to matomo.org should be fine (they were basically there before already). Also links to the developer website should be no problem @flamisz

@flamisz flamisz changed the title 16787 improve no data yet screen Improve no data tracked yet screen Mar 22, 2021
@flamisz flamisz marked this pull request as ready for review March 22, 2021 20:16
@flamisz flamisz added Needs Review PRs that need a code review not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. labels Mar 22, 2021
@flamisz flamisz added this to the 4.3.0 milestone Mar 22, 2021
diosmosis
diosmosis requested changes Mar 23, 2021
View reviewed changes
Copy link
Member

@diosmosis diosmosis left a comment

Choose a reason for hiding this comment

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

Tested locally, works, left a couple comments

plugins/CoreHome/angularjs/content-block/content-block.directive.js Outdated Show resolved Hide resolved
plugins/SitesManager/Controller.php Outdated Show resolved Hide resolved
plugins/SitesManager/Controller.php Outdated Show resolved Hide resolved
@diosmosis diosmosis removed the Needs Review PRs that need a code review label Mar 23, 2021
@flamisz flamisz added Needs Review PRs that need a code review and removed Needs Review PRs that need a code review labels Mar 24, 2021
@flamisz
Copy link
Contributor Author

flamisz commented Mar 24, 2021

I'll add the test for that cms type guess method.

@flamisz
Copy link
Contributor Author

flamisz commented Mar 25, 2021

As you can see there is a linked PR, matomo-org/tag-manager#307. I needed that, because inside the tabs, when we show the tagmanager code, without that change the user has to change the site and then change back, to show the containers.

But now I've found another issue. When the tabs are loaded, using the widget-loader and calls the tagmanager code includes the siteselector, and that throws an error from here:

matomo/plugins/CoreHome/angularjs/siteselector/siteselector.controller.js

Line 16 in eb1a6ba

$scope.model.loadInitialSites().then(function(){

I remembered @sgiehl modified that part a couple of weeks ago. If I modify back to the original code, it works. It looks like the issue is that...

Ok, nothing (but I comment it anyway 😄 ) I see there is a new commit from @sgiehl and it has been fixed...
Is it ok if I rebase my branch?

@diosmosis
Copy link
Member

Is it ok if I rebase my branch?

This would never be an issue 👍

@flamisz flamisz force-pushed the 16787-improve-no-data-yet-screen branch from 4c125f5 to b043c61 Compare March 26, 2021 02:53
@flamisz flamisz added the Needs Review PRs that need a code review label Mar 28, 2021
@flamisz flamisz requested a review from diosmosis March 28, 2021 19:01
@flamisz flamisz force-pushed the 16787-improve-no-data-yet-screen branch from 3e58d91 to 7473bd9 Compare March 28, 2021 23:56
@diosmosis diosmosis merged commit d26f304 into 4.x-dev Mar 29, 2021
@diosmosis diosmosis deleted the 16787-improve-no-data-yet-screen branch March 29, 2021 03:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@diosmosis diosmosis diosmosis requested changes

Assignees

@flamisz flamisz

Labels
Needs Review PRs that need a code review not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Milestone
4.3.0
Development

Successfully merging this pull request may close these issues.

Improve no data tracked yet please set up tracking code screen
3 participants
@flamisz @tsteur @diosmosis