-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
[WIP] Adding confirmation modal #179
Conversation
👍 |
Nice! I like your approach with macros. Once translations are added - good to merge for me. The JS will be redone probably once we select a js framework. I'd like to raise this discussion on upcoming IRC meeting (more info soon). |
@pjedrzejewski : I will add translation tomorrow! |
@pjedrzejewski : Added :p |
@Arn0d is |
@winzou : It worked when I tested (I will check again asap). There is not doc for |
We need to fix build in another PR, and update the scenario accordingly here too... Every delete action now requires confirmation step. |
@Arn0d Sorry I can't find out how the |
@winzou, no problem! look at the method show, line 23 |
@pjedrzejewski : I think I can update behat scenarios next week. |
@Arn0d and where did you get your |
@winzou : I wrote bootstrap-modal.js, I added Have you a javascript error ? |
can you rename it to something more specific? i think the default implementation of bootstrap-modal is already named that. If not, then this fantastic modal plugin is https://github.com/jschr/bootstrap-modal |
I think that javascript confirmation is not enough. Actions that are responsible for deleting resources should be accessible through POST request. But this aporeach requires form so this could be done with batch actions (becaues there is no sens to add form to each row).
What do you guys think about it? |
@jroberson : Rename? You speack about |
no i meann't rename bootstrap-modal.js to something else. |
@jrobeson : ahhhhh! ok! yep bootstrap-modal-confirmation.js ? |
sure. |
@norzechowicz All delete actions are accessible only via DELETE method. Unfortunately browsers do not always support this method so we use POST forms and use _method = DELETE parameter, which forces Symfony to accept the method. See here. |
@pjedrzejewski right! I should check it first ;P |
I have problems with Behat and Selenium. I renamed I am on Linux (Ubuntu, the latest), I just openJDK. What did I do wrong? Thanks |
I will try to check this when I find some time. Did you try htmlunit?
|
I did not use it... Do not you use selenium? |
In the Behat configuration you can find the following lines :
Behat ignore the JavaScript scenario by default, so I remove them from configuration but my test are still ignored. I don't Know why... Do Travis support JavaScript scenario? |
OK it works fine, the bootstrap-modal.js is indeed an extension and not the one taken from twitter :p |
@winzou : thanks but I need to update behat scenario... Did you already use selenium ? |
No I haven't, can't help that much. |
@norzechowicz :
The test : /**
* @Then /^I should open "([^"]*)" modal$/
*/
public function isOpenedModal($modal)
{
$locator = sprintf('#%s', $modal);
$modalContainer = $this->getSession()->getPage()->find('css', $locator);
$modalCssClasses = $modalContainer->getAttribute('class');
$this->assertSession()->elementExists('css', $locator);
echo $modalCssClasses;
if (!preg_match('/in/', $modalCssClasses)) {
throw new \Exception(
'The modal has not the css class "in"'
);
}
} |
@Arn0d Deleting taxonomy scenario is ignored because there is a failing scenario somewhere before. Try to fix it or mark it with tag
|
the test before Deleting taxonomy scenario is green ! There are only 2 tests ... |
zone.feature : OK @pjedrzejewski : can this PR could be merged? I got trouble because some tests (independent from deletion) does not pass. |
I forgot to change some buttons which does not use macro ... |
@Arn0d @pjedrzejewski What is the status of this PR? |
@winzou : I am waiting for feedback from Pawel! |
|
Ok Pawel! I will update ASAP! |
@pjedrzejewski : is it better? I can not really test them again. I got trouble on my Sylius installation.... |
@Arn0d Could you rebase this PR against master where all features are passing, if no, I can do this soon, just let me know. Thank you! :) |
I will try to rebase it before the end of the week (perhaps Thursday). Is it good for you? |
Travis missed this one hm... |
@pjedrzejewski I have restarted the Travis build, but after today's fail the have really long queues. |
@pjedrzejewski @stloyd : Did I make something wrong? Do you know what happen? |
@pjedrzejewski @stloyd if it's OK, shouldn't this be merged before your redesign ? |
@jjanvier Yes, I think this should be merged before, or included in that PR. @pjedrzejewski What do you think? |
@stloyd I can't merge it without our js scenarios running on saucelabs, which I'm trying to do nowww. |
Anything we can do to help this along? Can't let my customers into the admin area without this feature :) |
@Richtermeister I'm working on fallback to normal confirm page basing on this, if you want to help you can write some |
Correct the interface name for attribute subjects
I added confirmation modal based on bootstrap modal, it is applied on the delete button (defined in the button macro). For now, it miss the translations.