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

dev/core#606: Don't redirect to the Option Groups page when page is displayed in a modal #13312

Merged
merged 1 commit into from
Dec 18, 2018

Conversation

davialexandre
Copy link
Member

@davialexandre davialexandre commented Dec 18, 2018

Overview

This seems like a regression caused by #12473.

When the Option Group page is open in a modal and the user clicks the "Done" button, they're redirected to the Option Groups page and lose all the data they might have already entered:

Before

581

After

Clicking "Done" simply closes the modal:

581_fix

Also, when opened as a full page, it still works as intended by #12473:

581_page

Technical Details

The cause of the issue was the change in the button classes. There is no explanation for this change either in the PR or in the Gitlab issue, so I simply reverted it to the previously used class.

@civibot
Copy link

civibot bot commented Dec 18, 2018

(Standard links)

@civibot civibot bot added the master label Dec 18, 2018
@davialexandre davialexandre changed the title #dev/core/606: Don't redirect to the Option Groups page when page is displayed in a modal dev/core/#606: Don't redirect to the Option Groups page when page is displayed in a modal Dec 18, 2018
@davialexandre davialexandre changed the title dev/core/#606: Don't redirect to the Option Groups page when page is displayed in a modal dev/core#606: Don't redirect to the Option Groups page when page is displayed in a modal Dec 18, 2018
@davialexandre
Copy link
Member Author

cc @mattwire

@mattwire
Copy link
Contributor

Ok tested this and confirmed it is ok to merge. It is a regression from 5.5.0 though only affects editing option groups inline. @eileenmcnaughton This should probably go into the RC?

@colemanw colemanw changed the base branch from master to 5.9 December 18, 2018 12:53
@civibot civibot bot added 5.9 and removed master labels Dec 18, 2018
@colemanw
Copy link
Member

Rebasing for the RC, hang on...

@colemanw colemanw merged commit c345eef into civicrm:5.9 Dec 18, 2018
@eileenmcnaughton
Copy link
Contributor

@colemanw @totten this could possibly also go against 5.8 for a 5.8.3 release - although it's not recent enough to trigger a 5.8.3 release and we don't have anything else lined up that does qualify at this stage

@colemanw
Copy link
Member

I don't feel strongly about it but if someone wants to make a 5.8 pr I'll hit the merge button.

@eileenmcnaughton
Copy link
Contributor

OK -@colemanw - this should also be considered for an ESR 5.7.x release - which I assume you will do at some point

@davialexandre
Copy link
Member Author

Just following up on Eileen's comment above. Will this be considered for the esr, @colemanw?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants