Problem/Motivation
The "Add above" code JS relies on .clearfix selector to find the right DOM element.
This contains risk to break in forms with customization.
Proposed resolution
We should find a better selector, maybe enrich the DOM or seek a child and go one up if this is not possible.
Remaining tasks
Define approach first.
| Comment | File | Size | Author |
|---|---|---|---|
| #15 | 2954224-add-above.patch | 1.59 KB | mathilde_dumond |
| #13 | 2954224-13-add-above-button.patch | 1.4 KB | mathilde_dumond |
| #12 | 2954224-12--gin.patch | 1.46 KB | timohuisman |
| #12 | 2954224-12.patch | 1.44 KB | timohuisman |
| #10 | paragraphs-better_selector_clearfix-gin-2954224-10.patch | 655 bytes | gaards |
Comments
Comment #2
miro_dietikerSee also comments at #2946514-62: Add paragraph before button
Comment #3
johnchqueWorking on this. :)
Comment #4
johnchqueThis should work. :)
Comment #5
johnnydarkko commentedReroll of #4
Comment #6
pivica commentedThis will remove `.clearfix` selector which improves the current situation a bit. But i don't think it is a proper improvement in the long run because
1. The issue is stating that 'This contains risk to break in forms with customization.'. If we just remove the class but still keep `.siblings()` query we are still very dependant on HTML structure. I don't think that somebody will alter this structure that `.siblings()` will break but it can happen.
2. Having a div wrapper like `div.clearfix` just because of visual fix is a bad thing. This should be changed to `div.paragraph-type-add` or `div.paragraph-type-add-modal-wrapper`. Then we can add `clearfix` class to that div or decide to make stuff more flexible and apply proper CSS rules to `paragraph-type-add-modal-wrapper` CSS class. This will also help with CSS rules because we can not select that wrapper currently in a clean way.
Beside that naming of a lot of CSS classes is not that good and do not follow BEM syntax, but that is another problem and we can not refactor that easily now because of BC compatibility.
Comment #7
miro_dietikerYeah we discussed this, we can't really change all the CSS classes. Back then i asked for last changes and kept the discussion open. There was no consistent proposal ever that resulted in a more clear result. So this won't happen in the current major version. (Can't find the issue anymore, but we closed it then when we decided for stability.
I would love however to see us adding more structure where it is missing.
Comment #8
gaards commentedI made a reroll of #5 since the patch didn't apply on the latest version of Paragraphs due to changes made in https://www.drupal.org/node/3028468
Without this patch the "Add above" button isn't working in the Claro admin theme (clicking the button reloads the page instead of showing the modal).
Comment #9
bceyssenspatch #8 works for us in Claro theme!
Comment #10
gaards commentedThe Gin Admin Theme alters the markup compared to Claro by adding another wrapper, meaning the "Add above" button doesn't work in Gin with the patches provided here. I uploaded a patch (targeted against the latest stable Paragraphs release) to make it work with Gin, but this further highlights the need for a proper solution to make the add above functionality work in other admin themes besides Seven.
Comment #11
timme77 commented#10 works for me!
Comment #12
timohuismanRerolled patch #8 and #10 against 8.x-1.13. Because of the changes in https://www.drupal.org/node/3133439, there are now 2 places with the
siblings('.clearfix')selector.Note: I wasn't able to test the gin patch myself, but given the minimal change from the base patch, I'm assuming it works.
Comment #13
mathilde_dumond commentedThis new patch works on seven, gin and claro. I am though a bit unhappy with using the selector '.form-item' because I am afraid that this would not be specific enough, but I checked that it did work properly also when there is a container involved.
Comment #15
mathilde_dumond commentedWe poked around to find a common solution for several themes, but the structure was a bit too different (for the ones that we worked with: Seven, Claro, Gin and Stark), so we fell back on the '.clearfix or .form-action' solution.
Note that we tried to not use 'siblings', to work with more themes, but removing that meant that when you use containers, then you would find the 'add paragraph' button for your current table, but also all the containers inside of the table.
Comment #16
berdirComment #18
berdirThanks, committed.
Comment #21
gmarcel commentedAfter upgrading to the Paragraphs 1.15 release, the "Add above" button is no longer displayed with Gin Admin Theme enabled.
Previously with the Paragraphs 1.14 release I had applied the patch from #13 and that fixed the problem, unfortunately the patch from #15 does not fix this problem.
I use the currently latest Core version (9.4.5) and the latest Gin Admin Theme version (beta5).