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

Add a surrounding div to the plone.analytics viewlet [2.3] #219

Closed
wants to merge 2 commits into from

Conversation

erral
Copy link
Member

@erral erral commented Oct 7, 2019

Fixes #157 in Plone 4.3.x

@laulaz @idgserpro

@erral erral requested a review from laulaz October 7, 2019 06:49
@mister-roboto
Copy link

@erral thanks for creating this Pull Request and help improve Plone!

To ensure that these changes do not break other parts of Plone, the Plone test suite matrix needs to pass.

Whenever you feel that the pull request is ready to be tested, either start all jenkins jobs pull requests by yourself, or simply add a comment in this pull request stating:

@jenkins-plone-org please run jobs

With this simple comment all the jobs will be started automatically.

Happy hacking!

@erral
Copy link
Member Author

erral commented Oct 7, 2019

@jenkins-plone-org please run jobs

@erral
Copy link
Member Author

erral commented Oct 9, 2019

@jenkins-plone-org please run jobs

@jensens
Copy link
Member

jensens commented Oct 9, 2019

I am not sure if it is a good idea to change markup in Plone 4.3 these days. Other opinions?

@erral
Copy link
Member Author

erral commented Oct 9, 2019 via email

@idgserpro
Copy link

idgserpro commented Oct 9, 2019

I see no problem adding tests in Plone 4.3. Probably the tests are failing not because of the implementation in this PR.

@idgserpro
Copy link

I am not sure if it is a good idea to change markup in Plone 4.3 these days. Other opinions?

It's up to you and the release managers if this is doable.

Reading #157, if someone is using an implementation adding a id to the script tag, it will still work; And if it's not using it, plone-analytics will be rendered along the copy and past script analytics code.

In this scenario, we see no harm even in Plone 4.3.x, but some integrators may have a different opinion.

@jensens
Copy link
Member

jensens commented Oct 9, 2019

@idgserpro ok, I am a bit super-careful here. If you think it wont break, ok. But first tests need to go green.

@idgserpro
Copy link

ok, I am a bit super-careful here. If you think it wont break, ok. But first tests need to go green.

I agree. I'm just not sure they are really related to this change: we're having 59 errors in https://jenkins.plone.org/job/pull-request-4.3/18/#showFailuresLink

@erral maybe creating a new PR just changing a text file from https://github.com/plone/plone.app.layout/tree/2.3.x would show if these failures are really related to this change?

@erral
Copy link
Member Author

erral commented Oct 9, 2019

I have re-run the PR Jenkins job

@idgserpro
Copy link

@mauritsvanrees Since you've been the one responsible to give us Plone 4.3.19, do you think the changes here are ok, even changing the markup?

@mauritsvanrees
Copy link
Member

I tried it in the Plone 4.3 site of a customer, which uses a lot of Diazo. The result is that the snippet is lost: apparently the Diazo rules of this site cannot handle the change. That can be fixed in the theme of this site of course, but I don't want to force users of Plone 4.3 sites to check this and fix their themes. Or more likely: to discover after several months that no analytics have been gathered.

I see the possible use case, but for Plone 4.3 I would not do this.

@mauritsvanrees mauritsvanrees changed the title add a sorrounding div to the plone.analytics viewlet Add a surrounding div to the plone.analytics viewlet [2.3] Oct 10, 2019
@jensens jensens deleted the erral-issue-157-branch-2.3.x branch October 10, 2019 08:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants