Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Claro's node form's layout is handled by floats that were originally written for the Seven theme back in the dark ages of CSS.
This task is to refactor this to use either Grid of Flexbox. There should be no visual or functional differences. Care must be taken not to break focus order.
Comment | File | Size | Author |
---|---|---|---|
#31 | Screen Shot 2023-03-04 at 6.39.28 PM.png | 263.39 KB | smustgrave |
#31 | Screen Shot 2023-03-04 at 6.28.42 PM.png | 265.93 KB | smustgrave |
#30 | 3291549-30.patch | 4.32 KB | mherchel |
| |||
#29 | Screen Shot 2023-03-04 at 10.38.28.png | 140.94 KB | lauriii |
#25 | Screen Shot 2023-03-03 at 10.06.40.png | 174.37 KB | lauriii |
Issue fork drupal-3291549
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
wesruv CreditAttribution: wesruv as a volunteer commentedDibs!
Comment #3
wesruv CreditAttribution: wesruv as a volunteer commentedComment #4
Maninders CreditAttribution: Maninders at Srijan | A Material+ Company commentedHi @wesruv, I think you can not assign issues to your self, and it's long time you have assign this to your self.
Comment #5
Maninders CreditAttribution: Maninders at Srijan | A Material+ Company commentedHi Adding a patch for 'Refactor Claro's "node form" layout to not use floats' and for that also adding a before and after screenshots, so you can see the difference.
Comment #6
wesruv CreditAttribution: wesruv as a volunteer and commentedSo I reported this issue on Twitter, and was trying to figure out my first Drupal contribution with the help of Mike. I have a solution, but haven't had the time to figure out how to contribute it back.
I wanted to move this to CSS grid with @supports, and I was thinking I'd leave the float fallback because it works quite well going very far back. I'd argue this layout feels like a much better use case for grid, and it'd be better for sub themers and folks building on top of it to customize.
IMO having to use calc for width doesn't feel great, but it does work, I think grid would be much cleaner for the "future friendly" solution.
Also, TIL that flexbox respects RTL nicely, so that's fun.
Comment #7
wesruv CreditAttribution: wesruv as a volunteer and commentedHere's my solution, I'm overriding the float layout so IE11 and other crusty browsers can use that (it has nasty grid bugs), and good browsers will pick up the @supports which converts this to a grid layout.
Also overrode layout margin to use gap where it made sense to.
Comment #8
pradipmodh13 CreditAttribution: pradipmodh13 at Srijan | A Material+ Company for Drupal India Association commentedPatch #7 has been applied. It works fine using the support property.
Comment #9
smustgrave CreditAttribution: smustgrave at Mobomo commentedReviewed patch
Applied cleanly
As mentioned in the manual testing of #8 float none is being used
Tested at different breakpoints and all that still works.
LGTM
Comment #10
mherchelThis has an RTL issue where the sidebar is on the right hand side (should be on the left).
Also, I talked to @lauriii (core frontend framework manager), and he said if we're doing grid, we can do Drupal 10 only. There's little benefit to doing this in D9, as it works fine, and just adds complexity (we should be removing complexity). Moving the version to D10.
Some code comments below (tbh, I think it's a bit more complex than is needed).
Not sure if we need to use grid-areas here. It makes things a bit complicated.
IMO grid areas would make sense if we're using them theme-wide, but that's not the case here.
Grid automatically handles RTL. No need for specific styles.
Comment #11
Sakthivel M CreditAttribution: Sakthivel M at QED42 for Drupal India Association commented#11 Please review the drupal 10.x patch
Comment #13
mherchel@Sakthivel M Thanks for the patch
A couple notes:
1) If you're basing your patch off of another, please include an interdiff, so reviewers can easily see the differences.
2) Since we're doing a Drupal 10 patch (where all browsers support Grid), there's no need for the
@supports
rule.Also, since this file is PostCSS, feel free to use modern syntax that we now have in core such has nesting, CSS logical properties, etc.
Comment #14
ameymudras CreditAttribution: ameymudras at Salsa Digital commentedRefactored the above css not to use supports
Comment #15
mherchelThanks for the patch.
Still failing CI, and my feedback from #10 doesn't seem to be addressed.
Comment #16
Sakthivel M CreditAttribution: Sakthivel M at QED42 for Drupal India Association commented#16 Please verify the patch,
Fixed patch failed commands and removed @supports properties and used CSS logical properties.
Comment #17
Sakthivel M CreditAttribution: Sakthivel M at QED42 for Drupal India Association commented#17 Fixed patch failed commands
Comment #18
mherchelThe feedback from my comments in #10 are not yet addressed, and there's still floats used for layouts in the patch.
Comment #19
Gauravvvv CreditAttribution: Gauravvvv at Axelerant for Drupal India Association commentedComment #21
Gauravvvv CreditAttribution: Gauravvvv at Axelerant for Drupal India Association commentedComment #22
smustgrave CreditAttribution: smustgrave at Mobomo commentedNot sure if #10.2 has been addressed either.
Will need screenshots since .css file changed.
Comment #23
Gauravvvv CreditAttribution: Gauravvvv at Axelerant for Drupal India Association commentedI have added before/after patch screenshots for reference:
Before patch
After patch
Comment #24
smustgrave CreditAttribution: smustgrave at Mobomo commentedOnly nit picky maybe could be
.layout-region--node-footer .layout-region__content
but don't think that will hold up the issue.
Comment #25
lauriiiUnfortunately, this introduces some regressions with RTL.
Before:
After:
Comment #26
Anonymous (not verified) CreditAttribution: Anonymous at Axelerant for Drupal India Association commentedGaurav-drupal made their first commit to this issue’s fork.
Comment #27
Anonymous (not verified) CreditAttribution: Anonymous at Axelerant for Drupal India Association commentedI have fixed the RTL specific styling. please review
Comment #28
smustgrave CreditAttribution: smustgrave at Mobomo commentedConfirmed the issue on #25 has been addressed.
Comment #29
lauriiiThere's no margin on RTL 🤔
Comment #30
mherchelUploading this as a patch, since it's a radical departure from the MR above.
IMO, this CSS could be radically simplified.
Because there's multiple rows, it's much easier to use grid than flex
Both grid and flex have built in RTL support, we should make use of that using the gap property
I ended up refactoring this down to 32 lines of CSS (from 80 in the MR) and IMHO is easier to understand. PCSS is below (for better readability) and patch is attached.
Comment #31
smustgrave CreditAttribution: smustgrave at Mobomo commentedTesting patch #30 also seemed to work
Regular
RTL
Comment #33
lauriiiCommitted a832e39 and pushed to 10.1.x. Thanks!