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.

Comments

miro_dietiker created an issue. See original summary.

miro_dietiker’s picture

johnchque’s picture

Assigned: Unassigned » johnchque

Working on this. :)

johnchque’s picture

Status: Active » Needs review
StatusFileSize
new784 bytes

This should work. :)

johnnydarkko’s picture

Reroll of #4

pivica’s picture

This 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.

miro_dietiker’s picture

Status: Needs review » Needs work

do not follow BEM syntax

Yeah 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.

gaards’s picture

I 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).

bceyssens’s picture

patch #8 works for us in Claro theme!

gaards’s picture

The 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.

timme77’s picture

#10 works for me!

timohuisman’s picture

StatusFileSize
new1.44 KB
new1.46 KB

Rerolled 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.

mathilde_dumond’s picture

Status: Needs work » Needs review
StatusFileSize
new1.4 KB

This 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.

Status: Needs review » Needs work

The last submitted patch, 13: 2954224-13-add-above-button.patch, failed testing. View results

mathilde_dumond’s picture

StatusFileSize
new1.59 KB

We 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.

berdir’s picture

Status: Needs work » Needs review

The last submitted patch, 12: 2954224-12--gin.patch, failed testing. View results

berdir’s picture

Status: Needs review » Fixed

Thanks, committed.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

gmarcel’s picture

After 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).