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

Fixes #12106 - Show popovers correctly on interface modal #2822

Closed
wants to merge 1 commit into from

Conversation

tbrisker
Copy link
Member

When saving an interface the popovers were not destroyed, leading to
them breaking when opening the same interface form again.
Also, the help text for the various checkboxes has been hidden with
popover to reduce screen clutter.

@dLobatog
Copy link
Member

[test]

@dLobatog
Copy link
Member

@tbrisker Interesting! It fixes the issue well. The hidden help text admittedly looks better IMO, do you agree @ares @tstrachota ?

If you can, do you mind to write an integration test that checks the popovers work well after saving an interface and reopening it? It's kind of an obscure change that could very easily break.

Since the other part of this PR (the help text) is no-risk and already makes it better, would you submit a separate PR & issue number, etc.. with that? I think it'd be merged now already.

Thanks again!

When saving an interface the popovers were not destroyed, leading to
them breaking when opening the same interface form again.
@tbrisker
Copy link
Member Author

@dLobatog I have split the help text part to PR #2835 . However I'm not sure how to write an integration test for this - the popover is displayed but its position is miscalculated so it appears outside the screen.

@shlomizadok
Copy link
Member

Tested. Works very well 👍

@dLobatog
Copy link
Member

@tbrisker Fine, but I'll nag you if this comes as a regression later 😄

@dLobatog
Copy link
Member

Merged as 988cee4, thanks @tbrisker & @shlomizadok !

@dLobatog dLobatog closed this Oct 27, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants