Problem/Motivation
This is part of the CSS modernization initiative, and intended to be worked on by our Google Summer of Code student only.
Steps to reproduce
The stylesheet at https://git.drupalcode.org/project/drupal/-/blob/10.0.x/core/themes/claro/css/components/dropbutton.pcss.css needs to be refactored to make use of modern CSS and Drupal core's PostCSS tooling.
Proposed resolution
Use CSS Logical Properties where appropriate
Use CSS nesting where appropriate
Remaining tasks
We need two patches. One for Drupal 9.5.x and one for Drupal 10.0.x
We need a followup issue to refactor this component in Drupal 10.0.x to make use of component-level CSS custom properties and remove IE specific style definitions.
User interface changes
None. There should be no visual differences.
Comment | File | Size | Author |
---|---|---|---|
#51 | Screenshot at Dec 18 16-51-59.png | 90.64 KB | shweta__sharma |
#46 | 3303547-46.patch | 980 bytes | AkshayAdhav |
#45 | dropbutton-wrapper-11.x.png | 640.07 KB | AkshayAdhav |
#45 | dropbutton-wrapper-10.2.x.png | 597.63 KB | AkshayAdhav |
#42 | interdiff-38_42.txt | 942 bytes | Gauravvvv |
Issue fork drupal-3303547
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:
Comments
Comment #2
Aditya4478 CreditAttribution: Aditya4478 commentedComment #3
Aditya4478 CreditAttribution: Aditya4478 commentedComment #4
sasanikolic CreditAttribution: sasanikolic at Google Summer of Code commentedThis & at the end is not needed.
Let's first revert the rtl changes.
This is compiled to .dropbutton-widget .js .dropbutton-wrapper.open & {} but should instead be compiled to .js .dropbutton-wrapper.open .dropbutton-widget {}.
Same as above. Js should be the parent selector of .dropbutton.
I think this is a good file to start introducing better (local) css variables, but there is still an ongoing discussion on the naming convention in https://www.drupal.org/project/drupal/issues/3257287.
Comment #5
Sakthivel M CreditAttribution: Sakthivel M at QED42 for Drupal India Association commented#5 Please review the patch
Comment #6
ckrinaComment #7
ckrinaComment #8
shivam-kumar CreditAttribution: shivam-kumar as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedIt seems patch could not be applied to 10.1.x-dev. Needs to be worked for 10.1.x.
Comment #9
needs-review-queue-bot CreditAttribution: needs-review-queue-bot as a volunteer commentedThe Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. 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.
Comment #10
Gauravvvv CreditAttribution: Gauravvvv at Axelerant for Drupal India Association commentedComment #12
Gauravvvv CreditAttribution: Gauravvvv at Axelerant for Drupal India Association commentedComment #13
smustgrave CreditAttribution: smustgrave commentedMore nesting can be done for
.dropbutton__toggle
.js.no-touchevents
large file but a lot of those can be folded into each other.
Comment #14
starshapedComment #17
andy-blumOpened a new MR using @Gauravvvv and @starshaped's cherrypicked commits from !3547 to untangle a botched rebase. Going forward, as long as this issue doesn't get bumped to 10.2.x, contributors should be fine to merge origin/10.1.x back into this MR branch.
Comment #18
smustgrave CreditAttribution: smustgrave commentedThanks @andy-blum!
Comment #19
starshaped@smustgrave, I actually wasn't quite done with my refactoring and still need to fix a few issues, so I'm moving this back to needs work. Thanks @andy-blum for fixing my mess, I'll work off that new MR!
Comment #20
andy-blumAlso #17 is not in any way a review - just getting things back on track.
Comment #21
Santosh_Verma CreditAttribution: Santosh_Verma at Srijan | A Material+ Company for Drupal India Association commentedHi @starshaped
In the last commit (7a272525 - Cleaned up CSS) you removed logical property and wrote normal css in place of that.
But I think this is wrong approach as in the Proposed resolution they asked to Use CSS Logical Properties where appropriate. :)
Comment #22
starshaped@Santosh_Verma Yep, this was on purpose because the styles would not display correctly in RTL mode when using logical properties. Since using logical properties here caused a regression in the layout, I used non-logical properties instead.
Also, unassigning myself to this issue as I haven't had time to work on it--anyone, feel free to pick this one up!
Comment #23
Gauravvvv CreditAttribution: Gauravvvv at Axelerant for Drupal India Association commentedComment #24
smustgrave CreditAttribution: smustgrave commentedThis has changed a few times. Could we get before/after screenshots please. Also screenshots of rtl please.
Thanks.
Comment #25
Santosh_Verma CreditAttribution: Santosh_Verma at Srijan | A Material+ Company for Drupal India Association commentedI have tested the #MR3615
There are few error in the patch
error: patch failed: core/themes/claro/css/components/dropbutton.pcss.css:62
error: core/themes/claro/css/components/dropbutton.pcss.css: patch does not apply
Comment #27
Stanzin CreditAttribution: Stanzin as a volunteer commentedComment #28
Aditya4478 CreditAttribution: Aditya4478 commentedTry to attach screenshot too Stan
Patch LGTM !
Comment #29
nod_patch doesn't apply to 11.x
Comment #30
_utsavsharma CreditAttribution: _utsavsharma at OpenSense Labs for DrupalFit commentedPatch for 11.x.
Comment #31
_utsavsharma CreditAttribution: _utsavsharma at OpenSense Labs for DrupalFit commentedComment #32
kostyashupenkoCan't apply patch from #30, so did reroll again of the patch from #27 against 11.x
Comment #33
nod_The nesting is not right here. Resulting selector is
.dropbutton__item:first-of-type > * > *:active
Where it should beComment #34
_utsavsharma CreditAttribution: _utsavsharma at OpenSense Labs for DrupalFit commentedAddressed pointer in #33.
Comment #35
nod_There are a few other selectors that needs to be fixed, check out the diff on the generated css file, anything with
> * > *
looks suspicious and probably needs to be fixed.Comment #36
Gauravvvv CreditAttribution: Gauravvvv at Axelerant for Drupal India Association commentedAddressed #35, also fixed some logical properties. Attached interdiff for same.
Comment #37
smustgrave CreditAttribution: smustgrave commentedFor the screenshots but looking at the patch
Is this not overriding itself?
Comment #38
kostyashupenkoComment #39
kostyashupenkoComment #40
smustgrave CreditAttribution: smustgrave commentedBelieve this is good to go.
Comment #41
nod_Is this change intended?
Comment #42
Gauravvvv CreditAttribution: Gauravvvv at Axelerant for Drupal India Association commentedNo, this is not intended. I have fixed it. I have attached the patch and interdiff for same.
Comment #43
ckrinaCommitted 722c7c6 and pushed to 11.x. Thanks!
Comment #45
AkshayAdhavIn 10.2.x version the code for
.form-actions .dropbutton-wrapper
was:With this code, the dropdown had proper alignment.
In 11.x version it has been changed to:
And now it's breaking the dropdown alignment.
Steps to reproduce it:
- Install the contrib module Workbench Moderation
- Now after enabling the module edit any content type inside the structure and enable the moderation from the Manage moderation tab.
- Now create new content of the respective content type and now you can see the issue.
Comment #46
AkshayAdhavCreating a patch file instead of raising an MR as I am getting issues while creating a new branch from 11.x branch.
Comment #48
AkshayAdhavComment #49
shweta__sharma CreditAttribution: shweta__sharma at OpenSense Labs commentedHi @AkshayAdhav,
The alignment issue is handling in this - https://www.drupal.org/project/drupal/issues/3391800
Thanks
Comment #50
AkshayAdhavHi @shweta__sharma
The issue you have mentioned is about vertical displacement along with the issue of the button's text length.
As this particular issue was closed with wrong property values, it was adding left margin instead of right margin. You can check the screenshots attached in my comment #45.
Comment #51
shweta__sharma CreditAttribution: shweta__sharma at OpenSense Labs commentedOhh! Thanks, @AkshayAdhav for the clarity. I tested your MR 5859 on Drupal 11 the MR is cleanly applied and the issue is fixed. The space is added between buttons now. Attached screenshot for reference.
Comment #52
shweta__sharma CreditAttribution: shweta__sharma at OpenSense Labs commentedComment #53
quietone CreditAttribution: quietone at PreviousNext commentedI'm triaging RTBC issues. I read the IS and the comments.
This was committed to 11.x and then re-opened for a problem with the contrib module, Workbench Moderation. My first question is this reproducible with just Drupal core? The same comment mentions 10.2.x, so is this a problem only on 10.2?
I have found that opening Fixed issues makes it difficult to track history of problems, so the discussion from #45 onwards may need to move to a new issue.
Comment #55
nod_Committed 39fb7eb and pushed to 11.x. Thanks!
I opted to commit it from here since this is a simple mistake and it should have been part of the initial patch. I do not see it as a sign there is a deeper issue with the previous patch.
Comment #56
nod_and to anwser the questions.
The problem is not present in 10.2 because it doesn't have the refactor, 10.2 version was used as an example of what the code and behavior should be.
It is probably not possible to replicate with core since we got rid of dropbuttons on those kind of submits a while back.
Comment #57
quietone CreditAttribution: quietone at PreviousNext commented@nod_, thanks!