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.8] #218

Closed
wants to merge 2 commits into from

Conversation

erral
Copy link
Member

@erral erral commented Oct 7, 2019

Fixes #157 for Plone 5.1

@laulaz @idgserpro

@erral erral requested a review from laulaz October 7, 2019 06:48
@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

@idgserpro
Copy link

It may sound overkill, but I'm missing a test checking for this implementation. A simple python test checking for plone-analytics in browser.contents would suffice.

@erral
Copy link
Member Author

erral commented Oct 9, 2019

Test added.

@erral
Copy link
Member Author

erral commented Oct 9, 2019

@jenkins-plone-org please run jobs

@erral
Copy link
Member Author

erral commented Oct 9, 2019

This was already merged for Plone 5.2, we could do the same here, right? @jensens

@mauritsvanrees mauritsvanrees changed the title add a sorrounding div to the plone.analytics viewlet Add a surrounding div to the plone.analytics viewlet [2.8] Oct 10, 2019
@mauritsvanrees
Copy link
Member

It is pretty late in the game to change the html structure in Plone 5.1. See my comment just now for Plone 4.3.
This PR gives a small but useful new feature, but it causes possible breakage as well.

I would like Plone 5.1 releases to become boring. ;-)

Does anyone want to argue in favour of merging this in 5.1?

@jensens
Copy link
Member

jensens commented Oct 10, 2019

I share the same fear here. For 5.2 I merged it already and think it is fine.
But I would really like to hear more opinions from other developers for 5.1 and 4.3 below (I dont think there's any further 5.0.x planned.

@esteele @MrTango @davisagli @agitator @thet - Who else?

@mauritsvanrees
Copy link
Member

The merge on master has caused an unforeseen problem, which has been fixed, but a problem still remains (which might be a problem caused by the fix, not by your original PR, I don't know). See issue #227.
As said above, I would like Plone 5.1 releases to become boring. This does not sound boring. Sorry, but I will close this PR as well.

@jensens jensens deleted the erral-issue-157-branch-2.8.x branch January 12, 2021 15:06
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