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.

CommentFileSizeAuthor
#31 Screen Shot 2023-03-04 at 6.39.28 PM.png263.39 KBsmustgrave
#31 Screen Shot 2023-03-04 at 6.28.42 PM.png265.93 KBsmustgrave
#30 3291549-30.patch4.32 KBmherchel
#29 Screen Shot 2023-03-04 at 10.38.28.png140.94 KBlauriii
#25 Screen Shot 2023-03-03 at 10.06.40.png174.37 KBlauriii
#25 Screen Shot 2023-03-03 at 10.05.52.png187.54 KBlauriii
#23 Screenshot 2023-02-27 at 10.23.09 AM.png316.53 KBGauravvvv
#23 Screenshot 2023-02-27 at 10.22.01 AM.png317.29 KBGauravvvv
#17 reroll_diff_16_17.txt885 bytesSakthivel M
#17 3291549-17.patch7.51 KBSakthivel M
#16 reroll_diff_14_16.txt10.12 KBSakthivel M
#16 3291549-16.patch7.51 KBSakthivel M
#14 interdiff_11_14.txt8.46 KBameymudras
#14 3291549-14.patch4.83 KBameymudras
#11 3291549-11.patch5.38 KBSakthivel M
#10 node-form.png358.17 KBmherchel
#8 3291549-before-patch.png491.65 KBpradipmodh13
#8 3291549-before-patch.png491.65 KBpradipmodh13
#7 refactor-Claro-node-form-layout-to-not-use-floats-3291549-6.patch5.55 KBwesruv
#5 refactor-Claro-node-form-layout-to-not-use-floats-3291549-5-after.png488.63 KBManinders
#5 refactor-Claro-node-form-layout-to-not-use-floats-3291549-5-before.png493.01 KBManinders
#5 refactor-Claro-node-form-layout-to-not-use-floats-3291549-5.patch2.01 KBManinders
Cursor_and_Slack___mini_panel.png282.08 KBmherchel

Issue fork drupal-3291549

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:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mherchel created an issue. See original summary.

wesruv’s picture

Dibs!

wesruv’s picture

Assigned: Unassigned » wesruv
Maninders’s picture

Hi @wesruv, I think you can not assign issues to your self, and it's long time you have assign this to your self.

Maninders’s picture

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


wesruv’s picture

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

wesruv’s picture

Here'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.

pradipmodh13’s picture

Patch #7 has been applied. It works fine using the support property.

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

Reviewed 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

mherchel’s picture

Version: 9.5.x-dev » 10.0.x-dev
Status: Reviewed & tested by the community » Needs work
FileSize
358.17 KB

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

  1. +++ b/core/themes/claro/css/layout/node-add.pcss.css
    @@ -57,6 +57,64 @@
    +      grid-template-areas:
    

    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.

  2. +++ b/core/themes/claro/css/layout/node-add.pcss.css
    @@ -57,6 +57,64 @@
    +    [dir="rtl"] .layout-node-form {
    

    Grid automatically handles RTL. No need for specific styles.

Sakthivel M’s picture

Status: Needs work » Needs review
FileSize
5.38 KB

#11 Please review the drupal 10.x patch

Status: Needs review » Needs work

The last submitted patch, 11: 3291549-11.patch, failed testing. View results

mherchel’s picture

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

ameymudras’s picture

Status: Needs work » Needs review
FileSize
4.83 KB
8.46 KB

Refactored the above css not to use supports

mherchel’s picture

Status: Needs review » Needs work

Thanks for the patch.

Still failing CI, and my feedback from #10 doesn't seem to be addressed.

Sakthivel M’s picture

Status: Needs work » Needs review
FileSize
7.51 KB
10.12 KB

#16 Please verify the patch,

Fixed patch failed commands and removed @supports properties and used CSS logical properties.

Sakthivel M’s picture

#17 Fixed patch failed commands

mherchel’s picture

Status: Needs review » Needs work

The feedback from my comments in #10 are not yet addressed, and there's still floats used for layouts in the patch.

Gauravvvv’s picture

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

Gauravvvv’s picture

Status: Needs work » Needs review
smustgrave’s picture

Status: Needs review » Needs work
Issue tags: +Needs Review Queue Initiative, +Needs screenshots

Not sure if #10.2 has been addressed either.

Will need screenshots since .css file changed.

Gauravvvv’s picture

Status: Needs work » Needs review
FileSize
317.29 KB
316.53 KB

I have added before/after patch screenshots for reference:

Before patch

After patch

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs screenshots

Only nit picky maybe could be
.layout-region--node-footer .layout-region__content
but don't think that will hold up the issue.

lauriii’s picture

Issue summary: View changes
Status: Reviewed & tested by the community » Needs work
FileSize
187.54 KB
174.37 KB

Unfortunately, this introduces some regressions with RTL.

Before:

After:

Anonymous’s picture

Gaurav-drupal made their first commit to this issue’s fork.

Anonymous’s picture

Status: Needs work » Needs review

I have fixed the RTL specific styling. please review

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

Confirmed the issue on #25 has been addressed.

lauriii’s picture

Issue summary: View changes
Status: Reviewed & tested by the community » Needs work
FileSize
140.94 KB

There's no margin on RTL 🤔

mherchel’s picture

Status: Needs work » Needs review
FileSize
4.32 KB

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

/**
 * Layout overrides for node add/edit form.
 */
.layout-region {
  box-sizing: border-box;
}

.layout-region--node-footer .layout-region__content {
  margin-top: var(--space-l);
}

/**
 * Move to two column layout at wider widths.
 */
@media (min-width: 61rem) {
  .layout-node-form {
    display: grid;
    grid-template-columns: 3fr minmax(360px, 1fr);
    gap: var(--space-l);
  }

  .layout-region--node-main,
  .layout-region--node-footer {
    margin-inline: auto;
    width: min(832px, 100%);
  }

  /* Push sidebar down to horizontal align with form section. */
  .layout-region--node-secondary {
    margin-block-start: var(--space-l);
  }
}
smustgrave’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
265.93 KB
263.39 KB

Testing patch #30 also seemed to work

Regular

regular

RTL

rtl

  • lauriii committed a832e39b on 10.1.x
    Issue #3291549 by Gauravvvv, Sakthivel M, mherchel, Maninders,...
lauriii’s picture

Status: Reviewed & tested by the community » Fixed

Committed a832e39 and pushed to 10.1.x. Thanks!

Status: Fixed » Closed (fixed)

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