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

Added click-to-toggle for boolean columns #155

Closed
wants to merge 2 commits into from

Conversation

javiereguiluz
Copy link
Collaborator

This PR allows to click on any boolean value displayed in a listing to toggle its value: if you click on a YES value, you change it to NO and vice versa.

This is a WIP pull request to hear your opinions: I need to clean JavaScript code a bit, add PHPdoc, document the feature, etc.

I first saw this cool feature in SonataAdmin. I don't know if this feature is an original idea of Sonata project or if they copied it from someone else. In any case, thanks to whoever invented this feature and all the credits for the idea goes to him/her.


This is how it works

  1. If the boolean column corresponds to an entity property, you now can click on it to toggle its value:

toggle_1

  1. If the boolean column is a virtual field, you cannot click on it to toggle its value:

toggle_2

  1. When things go right, clicking on a boolean column toggles its value without having to edit the entity:

toggle_3

  1. If some error happens, the label shows a warning ERROR message until the page is reloaded:

toggle_4

@ogizanagi
Copy link
Contributor

It will require another js library, but what do you think of a real switch control like Bootstrap Switch or Bootstrap Toggle ? (For the error handling, it's really simple to add a switch-back behavior)
I do not find labels acting as controls very intuitive, and you cannot visually make the difference between virtual property and switchable fields.


$this->em->flush();

return new JsonResponse(array('success' => true));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As you're not using this response, and there is no other possible response content, you might just want to return an empty 200 or 204 ?

Or return the current boolean field value in the JsonResponse in order to properly change the control state if someone else updated it meanwhile:

User A requests the list page. Entity with id #50 has its boolean field set to true.
User B updates the entity with id #50 to false.
User A sees the entity with id #50 and its field as true and toggles it to false. In reality, this wil toggle it to true, as it was already changed to false before.
User A will think entity's field is set to false, but it's not.

Another way (certainly better) will be to send the new value in the Ajax request.

@javiereguiluz
Copy link
Collaborator Author

@ogizanagi about using a proper switch control, my answer may surprise you: I find most of those controls incredible unintuitive. If we implemente something like that, it would look like the following:

boolean_toggle

However, I'm still not convinced of this change.

@Pierstoval
Copy link
Contributor

I find this much more intuitive than the actual behavior, a switch design is much more valuable in UX terms IMO.

@ogizanagi
Copy link
Contributor

@javiereguiluz : Your solution is quite good as well. At least this looks like a control and not only a textual element.
As @Pierstoval said, relying on a simple label for a control is very poor and unintuitive in UX terms.

The buttons checkboxes/radios from Bootstrap should simply do the job too.

@sr972
Copy link
Contributor

sr972 commented Mar 6, 2015

TBH, i'm totally against this kind of change.

I'm just using checkboxes to display if a boolean value is set or not (e.g. with fontawesome and so on if given in project).

But imho it's a to big risk to make bools switchable that easy in an overview w/o getting a feedback or a question if i want to save this change. One little click and your app may be screwed or accounts blocked and so on. Just thinking about one of my closed source projects and if i would use EasyAdmin in it, if i would do 1 wrong click on 1 bool value, hundreds of devices would just stop working until manual interaction on the device.

I think you should make it at least not default. Let's make it an optional per field. An what about the security/rights to do this action? Could it be passed to? For example that the project lead could do this in the overview but not another person working with it?

@ogizanagi
Copy link
Contributor

@sr972 : The EasyAdmin bundle has nothing to do with rights, and IMO should keep it that way. I guess you can easily protect the action by checking the user's roles, overriding the AdminController::initialize method or using a request/controller listener or even directly in a voter.

What about a modal to confirm the action, or a banner advertising about the ongoing changes, and a button to submit all of them ?

@Pierstoval
Copy link
Contributor

@sr972 marks a point on security issues. I haven't thought about it, but after looking at the code, you're creating a new toggle action, which adds to the back-end complexity.

For the security issues, I think we should implement something, event a light feature, because security is prominent in many many back-ends, especially big ones. I will personally need this feature very much, (as well as custom actions, homepage and 2-level grouping) because one of my apps is divided into 3 apps : a "game", a "map" and a "portal" system, which will all have their own entities to manage. Plus, I'll need a special security "firewall" for some other roles, because some people will only have the rights to manage translations.

And even if "toggling" boolean values in the list is cool, it sounds more "gadget" (in french terms) than important features.

In fact, that's why I think that focusing on the roadmap might be preponderant. And as I said, I think that a toggle action for such a "little" feature is maybe too much.

@ogizanagi
Copy link
Contributor

@Pierstoval : Good point.
Do you have something in mind talking about security as a very light feature ? That's one of the reasons I'm in favor of decoupling every custom and build-in actions in the code in my vision of custom actions (which should not be different at all from build-in ones, IMO).

@Pierstoval
Copy link
Contributor

@ogizanagi Yup I got one: propose the user to use a different routing resource to allow him to put his backend under the access_control security component. I made a basic workaround for this in my URLS branch.
I rebased it many times (I always rebase my work on master) so it should fit to the master branch with no BC break, I think.

The goal is to propose the user to change its routing resource to the routing_pretty file, so the admin route becomes /{entity}/{action}/{id} , which is handle-able by the access_control component like this:

There's a discussion about this in #56, I had no answer from javier for now so I'm still waiting for it, but if we're looking for an easy way of handling security, this kind of feature sounds the lightest one, because it doesn't change the way the back-end works (because I re-set query string attributes in the controller, from the route ones, if they're set, and I force the list action if it's not set, so it's just one more redirection, which has no impact for the homepage)

@javiereguiluz
Copy link
Collaborator Author

I've listened to you and I've changed my mind. This feature now uses the bootstrap-toggle project and the results are better than expected.

1) Normal behavior

cool_toggle_1

2) An error occurred

We restore the original value and disable the toggle momentarily to avoid further issues.

cool_toggle_2

3) Race condition: two people change the same value at the same time and with different values

We always check the value which was persisted. If there is a mismatch, we automatically flip the toggle to always reflect the latest persisted value:

cool_toggle_3

@ogizanagi
Copy link
Contributor

👍 This is great

@javiereguiluz
Copy link
Collaborator Author

I'm going to merge this as it is now. However, I'm not ignoring your comments about security and I thank you all for raising concerns. We'll work on that later.

@javiereguiluz javiereguiluz deleted the toggle_boolean_fields branch March 7, 2015 09:10
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.

4 participants