Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Updated: Comment #38
Problem/Motivation
When multiple toolkits are available for selection in the Image toolkit form at admin/config/media/image-toolkit, all the detail setup of each toolkit is presented. This is regression from Drupal 7 where only the selected toolkit's details are presented.
Proposed resolution
Make visible only the details of the currently selected toolkit, and use the #states form property to make visible alternative toolkit settings upon change of the selection.
Remaining tasks
- Review patch.
User interface changes
API changes
Related Issues
Comment | File | Size | Author |
---|---|---|---|
#38 | interdiff_28-38.txt | 692 bytes | mondrake |
#38 | 2111263-38.patch | 6.69 KB | mondrake |
#29 | 2111263-28.patch | 6.72 KB | mondrake |
#29 | interdiff_21-28.txt | 4.52 KB | mondrake |
#21 | 2111263-21.patch | 8.4 KB | mondrake |
Comments
Comment #1
mondrakePatch.
Comment #2
mondrakeComment #3
claudiu.cristeaHm. Pure JS or should we try Ajax like in admin/config/content/formats/manage/basic_html (see WYSIWYG select)?
Comment #4
mondrakeWe can also do AJAX, even though I wonder if it is worth it (how many toolkits would a site install?). Let me know.
Comment #5
claudiu.cristeaNot necessary the number of toolkits but the details section will expand in #2110591: API/UI for toolkit operations plugin selection. As I see, now, inside toolkit details we'll have another details sections holding a list of all effects a toolkit is implementing. Where there is a competition (same operation & toolkit) there will be a select.
Comment #6
mondrakeOK. Attached an AJAX version.
Comment #7
claudiu.cristeaGreat!
@mondrake, I would like to play a little bit and test this manually. Do you have some alternative toolkit module just for purpose of this issue?
Comment #8
mondrakeI played a bit around a conversion of Imagemagick. It falls in the pitfalls we are highlighting in #2105863: [meta] Images, toolkits and operations, but it can do for the purpose of testing this issue.
You can pull the from the sandbox here.
Comment #9
claudiu.cristeaEDIT: Removed an initial comment.
Maybe
processToolkitDetails()
?Otherwise looks good :)
Comment #10
mondrakeOK. Here we go.
Comment #11
claudiu.cristeaI know I'm a little bit late (sorry!), but this seems to deserve a better doc/description.
Comment #12
mondrakeLike this?
Comment #13
claudiu.cristeaThank you. Looks good. RTBC if turns green.
Comment #14
alexpottLooks like we have 0 test coverage of this form
Comment #15
mondrakeAdded tests. In fact this was good as system.routing.yml was setting a non-existing 'administer administration pages' permission for this route. Corrected to 'access administration pages'.
Comment #16
mondrakeComment #17
mondrakeNoticed a few glitches.
Comment #18
claudiu.cristeaThank you for your work.
Maybe 'settings form' (singular)?
I think we should use
'#type' => 'details'
here. See https://drupal.org/node/1898824.$this->t()
insteadt()
."Contains \Drupal\system...". Leading backslash.
Always provided by parent class. Remove it.
This is no needed. The previous
$this->drupalGet()
is already checking for 200. See the test messages. Should be removed.Single line? https://drupal.org/coding-standards#array
It would be nice here if we can add some fake configs also to the 'test' toolkit. Then change the configs and test here to see if AJAX really worked. I mean if the changed configs get saved if they were exposed as a result of an AJAX load.
Also drop the message from
$this->assertEqual()
. We should not use custom messages in assertions.Comment #19
mondrakeThanks for review @claudiu.cristea
All done. In addition, added a warning message to the user to remind clicking on 'Save configuration' after a toolkit change in the form. The change processed by AJAX is not leading to a change in the config, it must be confirmed. With no message, users can be misled to believe the change is already effective. Similar behavior we have e.g. in the views UI.
Comment #20
claudiu.cristeaGood to go.
Comment #21
mondrakeNeeded reroll after #2080013: Remove Unused local variable $id from /core/modules/system/lib/Drupal/system/Form/ImageToolkitForm.php.
Comment #22
mondrakeBack to RTBC
Comment #23
alexpottAren't we doing js enhancement wrong here - shouldn't we use it to hide the forms for the unselected toolkits rather cause users without js all the extra page loads. I.e. shouldn't this work in the same way as the database selection form in the installer?
Comment #24
claudiu.cristeaThe 'details' part of the form for each toolkit will expand in the future. See #5. To be onest, I don't know exactly where is the limit between pure JS (aka 'hide') and Ajax. Anyway we want to add more per-toolkit elements in the this form.
Comment #25
webchickBack to alexpott, though not sure that his concern is addressed.
Comment #26
xjm21: 2111263-21.patch queued for re-testing.
Comment #27
claudiu.cristeaJust had a discussion with @alexpott on IRC related to this issue. We agreed to move back to JS form states as it was posted initially in #1. Sorry for all the work caused by my comment from #3.
@mondrake, do you think you can rework #21 with form states? Thank you!
Comment #28
mondrakeSure, no problem. We'll have to rethink how to process
getRequirements()
upon form validation though. But OK, one step at a time.Comment #29
mondrakeHere we go.
Comment #32
mondrake29: 2111263-28.patch queued for re-testing.
Comment #33
fietserwinRemove (and rename), see issue(s) #1852104: #type details: Remove #collapsible property (and #1892182: #type details: Rename #collapsed to #open) (or change the #type back to fieldset).
Comment #34
mondrakeI have updated the sandbox with the halfway conversion of the Imagemagick toolkit (see comment #8), to manually test the patch in #29. Just for info if anyone wants to test.
@fietserwin - re. #33: thanks, now I understand why setting #collapsible is totally irrelevant...
However, we have some contradictions here:
So what shall we do? My proposal is:
@fietserwin, @claudiu.cristea can I have your opinions before coding this?
Comment #35
claudiu.cristea@mondrake,
OK with all. Maybe #1892182: #type details: Rename #collapsed to #open will need a comment & "needs work" after this lands.
Comment #36
mondrakeOk. Tomorrow.
Comment #37
fietserwinI'm fine with the proposal of #34.
Note: I tested this manually by editing the image_test.info.yml file and setting hidden to false, then enabling the image_test module.
Comment #38
mondrakeImplements #34. The latest patch from #1892182: #type details: Rename #collapsed to #open already was doing a conversion of #collapsed to #open in ImageToolkitForm anyway, so I don't think we need to do anything more about it.
Updated issue summary to reflect we reverted from AJAX to JS states.
Comment #39
claudiu.cristeaGood to go if turns green.
Thank you!
Comment #40
alexpottCommitted 35d043d and pushed to 8.x. Thanks!