In node edit page, the checkbox is missing a left margin
In extension page, the checkbox is missing a left margin and should be verticaly aligned (top or middle)
see screenshots

CommentFileSizeAuthor
#22 ltr-fixed.png77.09 KBfhaeberle
#22 rtl-fixed.png59.44 KBfhaeberle
#22 ltr-broken.png61.03 KBfhaeberle
#22 rtl-broken.png61.4 KBfhaeberle
#20 claro-checkbox-modules-3047433-20.patch642 bytesfhaeberle
#17 claro-checkbox-modules-3047433-17.patch2.52 KBfhaeberle
#14 claro-checkbox-modules-3047433-14.patch1.75 KBfhaeberle
#6 node_edit_correct.png78.29 KBfhaeberle
#6 claro-checkbox-modules-3047433-6.patch978 bytesfhaeberle
node_edit.png18.08 KBdunebl
extension_page.png11.91 KBdunebl

Comments

DuneBL created an issue. See original summary.

huzooka’s picture

huzooka
Primary language Hungarian
Location Hungary 🇭🇺🇪🇺
commented

Re:

  1. Node edit page: we inherit a bad rule from @classy/entity-meta.css:
    .entity-meta__header .form-item {
    margin: 0.25em 0;
}

    That should change only vertical margins, so we'll replace it with margin-top: [something]; and margin-bottom: [something]; as the part of the issue #3023309: Accordion/Sidebar style update.

  2. System modules table: we have not touched that admin component yet, but we will. That issue should be solved there (when we'll have design for that).
huzooka’s picture

huzooka
Primary language Hungarian
Location Hungary 🇭🇺🇪🇺
commented
Related issues: +#3023309: Accordion/Sidebar style update
dunebl’s picture

dunebl
Primary language French
commented

This will be very nice!!

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Issue tags: +Needs upstream bugfix

This is a pre-existing bug in core. We should open an issue to the Drupal Core issue queue as well.

fhaeberle’s picture

fhaeberle
he/him
Location 🇩🇪 Stuttgart
commented
Status: Active » Needs review
StatusFileSize
new claro-checkbox-modules-3047433-6.patch978 bytes
new node_edit_correct.png78.29 KB

Could not reproduce the bug on /node/edit but corrected the spacing of the checkbox on the extension page which is removed to an rtl improvement on the table. It works now with rtl/ltr and the correct spacings.

dunebl’s picture

dunebl
Primary language French
commented

Patch is very nice but have no effect as it apply on the src folder and not on the dist folder.
Looking at the generated code, it is the dist folder which is used
<link rel="stylesheet" media="all" href="/themes/contrib/claro/css/dist/components/system-admin--modules.css?proyag" />
As a side note, I have no clue why there is such duplication of code:

/drupal_root/themes/contrib/claro/css/dist/components

/drupal_root/themes/contrib/claro/css/src/components

But when applying on the dist tree, the bug is fixed.

Regarding node/edit, you are right, the bug seems corrected with the last version.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented

@DuneBL Patch in #6 has been generated correctly. We use PostCSS to provide support for legacy browsers. More information can be found in the documentation.

fhaeberle’s picture

fhaeberle
he/him
Location 🇩🇪 Stuttgart
commented

@DuneBL I would suggest using an Code Editor like VS Code (feels good for frontend work) which shows you which files are modified/ignored by git to better understand a repository. You would then see that the /dist folder is hidden by git which means that you shouldn't normally edit it because it's ignored by cause. Would be great if somebody can review my changes in the patch.

dunebl’s picture

dunebl
Primary language French
commented

Thank you for the explanation...
I assume that I must run yarn build:css to get the change in the dist folder.
This is not the topic, but when doing this, I got the following error:

yarn run v1.15.2
$ cross-env NODE_ENV=production ./node_modules/.bin/postcss "css/src" --dir "css/dist" --base "css/src"
/bin/sh: 1: cross-env: Permission denied
error Command failed with exit code 126.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

Anyway, I have manually copied the change in the dist folder, and the bug is fixed.... But maybe it is better to use yarn.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented

@DuneBL I'm happy to help to solve that. Could you either ping me on Slack or open a support request in the Claro issue queue?

dunebl’s picture

dunebl
Primary language French
commented
Status: Needs review » Reviewed & tested by the community
huzooka’s picture

huzooka
Primary language Hungarian
Location Hungary 🇭🇺🇪🇺
commented
Status: Reviewed & tested by the community » Needs work

Re #6:

+++ b/css/src/components/system-admin--modules.css
@@ -10,9 +10,13 @@
+  padding-left: 1rem;

Nitpick: add comment /* LTR */ here.

fhaeberle’s picture

fhaeberle
he/him
Location 🇩🇪 Stuttgart
commented
Assigned: Unassigned » fhaeberle
Status: Needs work » Needs review
StatusFileSize
new claro-checkbox-modules-3047433-14.patch1.75 KB

Adding the code comment suggested in #13.

fhaeberle’s picture

fhaeberle
he/him
Location 🇩🇪 Stuttgart
commented
Assigned: fhaeberle » Unassigned
huzooka’s picture

huzooka
Primary language Hungarian
Location Hungary 🇭🇺🇪🇺
commented
Status: Needs review » Needs work 
+++ b/css/src/components/system-admin--modules.css
@@ -10,7 +10,11 @@
+<<<<<<< Updated upstream
...
+=======
...
+>>>>>>> Stashed changes

This is an unresolved conflict, please fix it!

fhaeberle’s picture

fhaeberle
he/him
Location 🇩🇪 Stuttgart
commented
StatusFileSize
new claro-checkbox-modules-3047433-17.patch2.52 KB

Sorry for that mistake. Should be resolved now.

fhaeberle’s picture

fhaeberle
he/him
Location 🇩🇪 Stuttgart
commented
Status: Needs work » Needs review
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented

@fhaeberle it seems like there's some additional output in your patch. Please make sure to read the instructions on how to generate a patch for Drupal.

fhaeberle’s picture

fhaeberle
he/him
Location 🇩🇪 Stuttgart
commented
StatusFileSize
new claro-checkbox-modules-3047433-20.patch642 bytes

Sorry, I'm new to drupal contributing with the patch workflow. A bit confused about the possibilities here. Thanks for pointing me to the right source. I see, I provided a multi-patch, but we require a normal patch here. Here it is. :)

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented

@fhaeberle thank you! The patch looks better now!

Would it be possible to get a before and after screenshot? I'm not sure if I can reproduce the problem anymore.

fhaeberle’s picture

fhaeberle
he/him
Location 🇩🇪 Stuttgart
commented
StatusFileSize
new rtl-broken.png61.4 KB
new ltr-broken.png61.03 KB
new rtl-fixed.png59.44 KB
new ltr-fixed.png77.09 KB

For sure. Provided screenshots for the problem, screenshots are made with the mobile view but does also appear on the larger desktop viewport. Both ltr/rtl.

huzooka’s picture

huzooka
Primary language Hungarian
Location Hungary 🇭🇺🇪🇺
commented
Status: Needs review » Reviewed & tested by the community

I think that #20 is fine.

  • lauriii committed 81487fe on 8.x-1.x 
    Issue #3047433 by fhaeberle, DuneBL, huzooka: Check box input element...
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs upstream bugfix

Committed and pushed. Thank you!

Status: Fixed » Closed (fixed)

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