Steps:
1) Navigate to Structure> Taxonomy> Add Vocabulary
2) Observe the screenshot

Expected: Proper spacing is required between the text box and the text
Actual: The 'M' of 'Machine name' overlaps with the green border around the text box.

Before

After

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Neetika K created an issue. See original summary.

fly2ganesh’s picture

Assigned: Unassigned » fly2ganesh
fly2ganesh’s picture

fly2ganesh’s picture

Assigned: Unassigned » fly2ganesh
Status: Active » Needs review
andrewmacpherson’s picture

Project: Claro » Drupal core
Version: 8.x-1.0-beta1 » 8.9.x-dev
Component: User interface » Claro theme
Assigned: fly2ganesh » Unassigned
Status: Needs review » Needs work

Thanks for the patch @imganesh.

Review of patch #3:

  1. Claro is included in Drupal core as an experimental theme now, so patches need to be made against the Drupal core repo.
  2. diff --git a/css/src/components/form.css b/css/src/components/form.css
    The CSS src directory isn't used anymore, since #3088805: Simplify Claro's CSS directory structure.
  3. Changes should be made to the PostCSS file (form.pcss.css). Instructions are in Drupal core using PostCSS for development
  4. +.form-item__suffix .machine-name-label {
    I wonder if this can be generalized to .form-item__suffix? There will be other cases where this happens. In live websites, number fields will often have a suffix.
  5. + margin-left: 0.75rem;
    Does it need to be as much as this? I tried in the browser dev tools, and 0.25rem was enough to clear the green focus indicator.
fly2ganesh’s picture

Assigned: Unassigned » fly2ganesh
fly2ganesh’s picture

I agree with your point for making generalized to `.form-item__suffix`. I tested this issue with other cases and it was reproducible.

fly2ganesh’s picture

Status: Needs work » Needs review

I agree with your point for making generalized to `.form-item__suffix`. I tested this issue with other cases and it was reproducible.

Devipriya Rajamanickam’s picture

Status: Needs review » Needs work
FileSize
25.24 KB

I have applied patch #7. But still I see the overlapping of 'M' with the green border around text box. I have attached screenshot also for reference.

ant1’s picture

@imganesh: I believe you forgot to compile the CSS. Check the documentation of Front-end developer tools in core.

fly2ganesh’s picture

Thanks @antoineh, I have updated the patch with compiled CSS.

fly2ganesh’s picture

Status: Needs work » Needs review

Thanks @antoineh, I have updated the patch with compiled CSS.

ant1’s picture

Status: Needs review » Needs work
FileSize
2.31 KB
2.4 KB

Looks good.

Suffix layout after applied patch

Some comments to consider:

  • The margin should only be applied when the suffix is behind the input field. On mobile screens this is not the case (I believe 600px is the breakpoint where the layout switches). See image under list for reference of the issue.
  • Maybe we should add a comment why this margin is being applied.
  • A personal nitpick: maybe we could use the --space-xs variable (equals to 0.5rem). We have these defined so why not use them :)

Suffix layout after applied patch

ant1’s picture

Assigned: fly2ganesh » Unassigned
Status: Needs work » Needs review
FileSize
1.01 KB
1.06 KB
2.39 KB
2.45 KB

Addressed notes of comment #13.

Adding field after patch
Adding field on mobile after patch

ant1’s picture

KondratievaS’s picture

FileSize
86.88 KB

Tested patch from #14 for mobile and desktop resolutions in Chrome, FF, Safari and IE browsers. Result is OK

ok

KondratievaS’s picture

Status: Needs review » Reviewed & tested by the community
jungle’s picture

Status: Reviewed & tested by the community » Needs work

31 coding standards messages

Coding standards check must pass, see https://www.drupal.org/pift-ci-job/1614456

jungle’s picture

Status: Needs work » Reviewed & tested by the community

Maybe #18 out of scope there.

apaderno’s picture

What reported as coding standard warning for the form.pcss.css file are false positives caused from the fact the coding standard rules aren't able to handle .pcss files. (What is valid syntax for a .pcss file is not valid syntax for a .css file.)

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

xjm’s picture

Issue summary: View changes

Embedding the "before" and "after" screenshots in the IS for easier review.

xjm’s picture

xjm’s picture

ckrina’s picture

On a design perspective the change is 👍

lauriii’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: -Needs frontend framework manager review
+++ b/core/themes/claro/css/components/form.css
@@ -189,6 +189,14 @@ tr .form-item,
+    margin-left: 0.5rem;

+++ b/core/themes/claro/css/components/form.pcss.css
@@ -114,6 +114,13 @@ tr .form-item,
+    margin-left: var(--space-xs);

We should add RTL styles for this 🧐

boulaffasae’s picture

Assigned: Unassigned » boulaffasae
boulaffasae’s picture

boulaffasae’s picture

Assigned: boulaffasae » Unassigned
Status: Needs work » Needs review
KondratievaS’s picture

FileSize
60.97 KB

Tested patch from #28 for mobile and desktop resolutions in Chrome, FF, Safari and IE browsers. Result is OK

OK

KondratievaS’s picture

Status: Needs review » Reviewed & tested by the community
xjm’s picture

Tagging for @lauriii's review again. Thanks for the updated patch!

lauriii’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: -Needs frontend framework manager review
  1. +++ b/core/themes/claro/css/components/form.pcss.css
    @@ -114,6 +114,17 @@ tr .form-item,
    +    margin-left: var(--space-xs);
    

    Let's add comment here to highlight that this is for LTR https://www.drupal.org/docs/develop/standards/css/css-formatting-guideli...

  2. +++ b/core/themes/claro/css/components/form.pcss.css
    @@ -114,6 +114,17 @@ tr .form-item,
    +  [dir="rtl"] .form-item__suffix {
    +    margin-right: var(--space-xs);
    +  }
    

    We should reset the margin-left here so that both, LTR and RTL behave in the same way.

sauravk’s picture

Assigned: Unassigned » sauravk
sauravk’s picture

Assigned: sauravk » Unassigned
Status: Needs work » Needs review
FileSize
1.2 KB
753 bytes
KondratievaS’s picture

FileSize
88.25 KB

Tested patch from #35 for mobile and desktop resolutions in Chrome, FF, Safari and IE browsers. Result is OK

OK

KondratievaS’s picture

Status: Needs review » Reviewed & tested by the community
boulaffasae’s picture

Assigned: Unassigned » boulaffasae
Status: Reviewed & tested by the community » Needs work

I think we missed the 1 point by laurii

+++ b/core/themes/claro/css/components/form.pcss.css
@@ -114,6 +114,17 @@ tr .form-item,
+    margin-left: var(--space-xs);

Let's add comment here to highlight that this is for LTR https://www.drupal.org/docs/develop/standards/css/css-formatting-guideli...

boulaffasae’s picture

Assigned: boulaffasae » Unassigned
Status: Needs work » Needs review
FileSize
1.22 KB
875 bytes

LTR comment added

atul4drupal’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
120.82 KB
261.61 KB

tested #39 which is slight improvement from #35 with respect to format standardization.

It is working as expected.

atul4drupal’s picture

Going by the practice tagging issue to be review by front-end manager

  • lauriii committed beee042 on 9.1.x
    Issue #3097540 by imganesh, boulaffasae, antoineh, sauravk, KondratievaS...
lauriii’s picture

Version: 9.1.x-dev » 9.0.x-dev
Issue tags: -Needs frontend framework manager review
FileSize
1006 bytes

Since the input element is set as display block with max-width: 600px, we should use min-width: 601px here. This is such a small change so I made the change on commit.

Committed beee042 and pushed to 9.1.x. We could probably port this to 9.0.x and 8.9.x but will confirm with other committers. Thanks!

  • xjm committed aaed374 on 9.0.x
    Issue #3097540 by imganesh, boulaffasae, antoineh, sauravk, KondratievaS...

  • xjm committed be21de7 on 8.9.x
    Issue #3097540 by imganesh, boulaffasae, antoineh, sauravk, KondratievaS...
xjm’s picture

Version: 9.0.x-dev » 8.9.x-dev
Priority: Minor » Normal
Status: Reviewed & tested by the community » Fixed

Given that Claro is in beta, and the bugfix is not all that disruptive, I think it is safe to backport. Applied #39 as well as @lauriii's interdiff from #43 to 9.0.x, committed, and cherry-picked to 8.9.x. Nice work!

Status: Fixed » Closed (fixed)

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