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

Some confirms are broken in 3.x #10479

Closed
sgiehl opened this issue Sep 7, 2016 · 3 comments · Fixed by #10494
Closed

Some confirms are broken in 3.x #10479

sgiehl opened this issue Sep 7, 2016 · 3 comments · Fixed by #10494
Assignees
@sgiehl
Labels
answered For when a question was asked and we referred to forum or answered it. Regression Indicates a feature used to work in a certain way but it no longer does even though it should.
Milestone
3.0.0-b1

Comments

@sgiehl
Copy link
Member

sgiehl commented Sep 7, 2016

When opening a modal confirm for a specific element, the element is currently moved into the modal box. Closing the box, doesn't move it back, so recreating won't work, as the content can't be found anymore.

To reproduce the failure, simply try to remove a segment, click no, and try it again.

Cloning the content before showning didn't work. See #10478
@sgiehl sgiehl added the Regression Indicates a feature used to work in a certain way but it no longer does even though it should. label Sep 7, 2016
@sgiehl sgiehl added this to the 3.0.0-b1 milestone Sep 7, 2016
@tsteur
Copy link
Member

tsteur commented Sep 13, 2016

I had this tested and it works fine for me but can reproduce it for "remove segment". We should have a look why it fails for this confirm but not for others. I'll have a quick look

@tsteur
Copy link
Member

tsteur commented Sep 13, 2016

Problem is that it uses here https://github.com/piwik/piwik/blob/3.x-dev/plugins/SegmentEditor/javascripts/Segmentation.js#L682

self.target.find('.segment-delete-confirm') instead of eg $('.segment-delete-confirm'). We usually address them via an id I think like $('#resetDashboardConfirm'). Maybe we can simply replace self.target.find with $?

@sgiehl
Copy link
Member Author

sgiehl commented Sep 13, 2016

should work I guess. Maybe we should give the dialog an id, so it's consistent with other dialogs.

@sgiehl sgiehl self-assigned this Sep 13, 2016
sgiehl added a commit that referenced this issue Sep 13, 2016
@sgiehl
fixes #10479 use id attribute to remove segment dialog
378e8d7
@sgiehl sgiehl mentioned this issue Sep 13, 2016
Merged
@sgiehl sgiehl closed this as completed Sep 13, 2016
@mattab mattab added the answered For when a question was asked and we referred to forum or answered it. label Oct 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@sgiehl sgiehl

Labels
answered For when a question was asked and we referred to forum or answered it. Regression Indicates a feature used to work in a certain way but it no longer does even though it should.
Projects
None yet
Milestone
3.0.0-b1
Development

Successfully merging a pull request may close this issue.

Fix remove segment confirm dialog matomo-org/matomo
3 participants
@tsteur @mattab @sgiehl