Closed (fixed)
Project:
Drupal core
Version:
10.1.x-dev
Component:
Claro theme
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Issue tags:
Reporter:
Created:
20 Jun 2022 at 18:14 UTC
Updated:
19 Mar 2023 at 08:14 UTC
Jump to comment: Most recent, Most recent file

Comments
Comment #2
wesruv commentedDibs!
Comment #3
wesruv commentedComment #4
maninders 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 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 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 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 commentedPatch #7 has been applied. It works fine using the support property.
Comment #9
smustgrave 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 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
@supportsrule.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 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 commented#16 Please verify the patch,
Fixed patch failed commands and removed @supports properties and used CSS logical properties.
Comment #17
sakthivel m 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 commentedComment #21
gauravvvv commentedComment #22
smustgrave commentedNot sure if #10.2 has been addressed either.
Will need screenshots since .css file changed.
Comment #23
gauravvvv commentedI have added before/after patch screenshots for reference:
Before patch

After patch

Comment #24
smustgrave commentedOnly nit picky maybe could be
.layout-region--node-footer .layout-region__contentbut 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) commentedGaurav-drupal made their first commit to this issue’s fork.
Comment #27
Anonymous (not verified) commentedI have fixed the RTL specific styling. please review
Comment #28
smustgrave 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 commentedTesting patch #30 also seemed to work
Regular
RTL
Comment #33
lauriiiCommitted a832e39 and pushed to 10.1.x. Thanks!