Problem/Motivation

Dropbuttons on forms in conjunction with regular buttons can be misaligned due to discrepancy on the margins top and bottom.

Steps to reproduce

An example is a Feed Channel entity as provided by the Feeds module on a Drupal 9 install with Gin theme active.

The Feed Channel entity features a dropbutton + a regular button.

This can reproduced both using Claro and Gin, who depends on Claro:

Only local images are allowed.

Misaligned dropbutton and button

Proposed resolution

The .dropbutton-wrapper CSS that sets margin top and bottom to 0.5rem is provided by Claro
/core/themes/claro/css/components/dropbutton.css?rq2mag

The 1rem margin for .form-actions .action-link is also provided by Claro on core/themes/claro/css/components/form.css

They should be normalized.

Remaining tasks

Decide a solution and implement it.

User interface changes

None

API changes

None

Data model changes

None

CommentFileSizeAuthor
#27 dropButtonFix.png16.2 KBohjoz
#25 after-patch-claro.png17.27 KBjennakoo
#21 3341669-21-9.5.x.patch958 bytesthomwilhelm
#19 AfterPtach.png4.98 KBnishant
#19 BeforePatch.png5.17 KBnishant
#18 3341669_18.patch1.18 KBathyamvidyasagar
#15 Pass -claro - After unticking css and js aggregation.png5.22 KBkunal_sahu
#15 Css not aggregated with patch changes.png160.66 KBkunal_sahu
#15 css file changes after patch applied.png6.65 KBkunal_sahu
#15 claro theme - fail - Patch doesnt aggregate directly on Claro theme css.png50.77 KBkunal_sahu
#11 3341669-nr-bot.txt91 bytesneeds-review-queue-bot
#9 3341669-9-10.1.x.patch978 bytesgauravvvv
#5 3341669-after-patch-claro.png18.05 KBnayana_mvr
#5 3341669-after-patch-gin.png20.91 KBnayana_mvr
#5 3341669-before-patch-claro.png18.2 KBnayana_mvr
#5 3341669-before-patch-gin.png21.58 KBnayana_mvr
Screenshot_20230214_165856.png5.22 KBidiaz.roncero

Issue fork drupal-3341669

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

idiaz.roncero created an issue. See original summary.

royalpinto007’s picture

royalpinto007
he/him
Primary language English
commented
Assigned: Unassigned » royalpinto007

royalpinto007 opened merge request !3470

royalpinto007’s picture

royalpinto007
he/him
Primary language English
commented
Status: Active » Needs review

I updated the margin values of .form-actions .action-link to match the 0.5rem margin used for .dropbutton-wrapper in the Claro theme's dropbutton.css file. This will ensure that the margins are consistent across both types of buttons.

We can even think of updating the margin values of the .dropbutton-wrapper CSS to match the 1rem margin used for .form-actions .action-link in the Claro theme's form.css file.

nayana_mvr’s picture

nayana_mvr commented
StatusFileSize
new 3341669-before-patch-gin.png21.58 KB
new 3341669-before-patch-claro.png18.2 KB
new 3341669-after-patch-gin.png20.91 KB
new 3341669-after-patch-claro.png18.05 KB

Verified the MR!3470 of #3 and tested it on Drupal version 9.5.x. The patch works fine and I have added the before and after screenshots for reference.

royalpinto007’s picture

royalpinto007
he/him
Primary language English
commented
Assigned: royalpinto007 » Unassigned
smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative

Change does seem to fix the issue.

rohan-sinha’s picture

rohan-sinha commented

Verified the merge request on #3, issue has been fixed.

gauravvvv’s picture

gauravvvv commented
StatusFileSize
new 3341669-9-10.1.x.patch978 bytes

Attached patch for 10.1.x

nayana_mvr’s picture

nayana_mvr commented

Verified the patch #9 and tested it on Drupal version 10.1.x. The patch works fine and I can confirm that the issue is resolved in Drupal 10 version also. Screenshots are same as in #5.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new 3341669-nr-bot.txt91 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

nod_’s picture

nod_
Primary language French
Location Lille
commented
Version: 9.5.x-dev » 10.1.x-dev
Status: Needs work » Needs review

Patch ans issue version mismatch

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Putting back to RTBC then.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 9: 3341669-9-10.1.x.patch, failed testing. View results

kunal_sahu’s picture

kunal_sahu commented
StatusFileSize
new claro theme - fail - Patch doesnt aggregate directly on Claro theme css.png50.77 KB
new css file changes after patch applied.png6.65 KB
new Css not aggregated with patch changes.png160.66 KB
new Pass -claro - After unticking css and js aggregation.png5.22 KB

Hi I tested the above patch 3341669-9-10.1.x.patch , on gin theme. It applied successfully.

PS C:\xampp\htdocs\drupal-3341669> git apply -v .\3341669-9-10.1.x.patch
Checking patch core/themes/claro/css/components/form.css...
Checking patch core/themes/claro/css/components/form.pcss.css...
Applied patch core/themes/claro/css/components/form.css cleanly.
Applied patch core/themes/claro/css/components/form.pcss.css cleanly.
PS C:\xampp\htdocs\drupal-3341669> composer require drupal/gin

I verified it on Gin theme , it works perfectly fine as per the issuefix.

But when i verified it on Claro , seems the patch doesn't work for Claro (Directly). Attaching Screenshots for both.

After some research , i could find that , the patch if applied properly can work in give us the solution , but it seems that the aggregated css which is generated by drupal is not having the changes from the patch which are applied in form.css file.

And after a certain amount of research , i could understand that while clearing the cache from config/development/performance we get and option which is by default ticked to have Aggregated css and JS. So we have to untick both the option and let the css aggregate again.

Attaching ss for all .

The patch LGTM.

But just the part of aggregation needs to be taken care of else. Everything looks good to me.

RTBC +1 for #9

Thanks

gauravvvv’s picture

gauravvvv commented
Status: Needs work » Reviewed & tested by the community

Unrelated failure, restoring status

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 9: 3341669-9-10.1.x.patch, failed testing. View results

athyamvidyasagar’s picture

athyamvidyasagar commented
StatusFileSize
new 3341669_18.patch1.18 KB

I have created this patch for 10.1.x-dev and its working as expected. Please verify

nishant’s picture

nishant
Primary language English
commented
StatusFileSize
new BeforePatch.png5.17 KB
new AfterPtach.png4.98 KB

Applied #18 patch on Drupal 10.1.x-dev with Claro theme and It's working fine.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

thomwilhelm’s picture

thomwilhelm commented
StatusFileSize
new 3341669-21-9.5.x.patch958 bytes

Supplying a 9.5.x version of the patch for anyone upgrading to Claro before the Drupal 10 upgrade.

Edit: Apologies the same fix doesn't work on 9.5.x for me, apologies for the noise.

thomwilhelm’s picture

thomwilhelm commented

jennakoo made their first commit to this issue’s fork.

jennakoo opened merge request !5329

jennakoo’s picture

jennakoo commented
StatusFileSize
new after-patch-claro.png17.27 KB

Copied the fixing lines from MR!3470 of #3 and reapplied on Drupal 11.x.

jennakoo’s picture

jennakoo commented
Status: Needs work » Needs review
ohjoz’s picture

ohjoz commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new dropButtonFix.png16.2 KB

Reviewed the code and tested the latest patch against 11.x, text is now aligned.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Reviewed & tested by the community » Needs work

Nice work here! I posted a minor comment on the MR regarding duplicate properties.

jennakoo’s picture

jennakoo commented
Status: Needs work » Needs review

Fixed the change requested by laurii.

laurielim’s picture

laurielim commented
Status: Needs review » Reviewed & tested by the community

Tested change and reviewed code, duplicate margin property removed.

  • lauriii committed 33b96890 on 11.x 
    Issue #3341669 by jennakoo, royalpinto007, ThomWilhelm, Gauravvvv,...

  • lauriii committed 9ab82dfb on 10.2.x 
    Issue #3341669 by jennakoo, royalpinto007, ThomWilhelm, Gauravvvv,...
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Version: 11.x-dev » 10.2.x-dev
Status: Reviewed & tested by the community » Fixed

Committed 33b9689 and pushed to 11.x. Also cherry-picked to 10.2.x. Thanks!

lauriii closed merge request !5329

Status: Fixed » Closed (fixed)

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