Refactor Claro's file stylesheet

Patch for Drupal 10.0.x

please completely ignore this.

Do not consider this patch

For D9 there are now changes found. For D10 please consider this patch.

"--positioning-elements" sounds good ?

Trial Patch.

Practicing

hello @sasanikolic,
It was a trial patch as this was my first contribution in drupal, just getting hands on experience with drupal 9/10.

Are they any more issues opened related to CSS modernization or some frontend development where i can potentially contribute to?

@hitaarthh Welcome! If you are interested in helping on CSS refactoring, we have many CSS refactoring issues in the Claro issue queue. Here's a link for the Claro issue queue: https://www.drupal.org/project/issues/drupal?component=Claro+theme

+++ b/core/themes/claro/css/components/file.pcss.css
@@ -4,16 +4,15 @@
+  padding-inline-start: var(--space-l); /* LTR */

Should we use this in both Drupal 10 and Drupal 9 since this would get compiled by PostCSS?

We can also remove the LTR comment since this now applies for both, LTR and RTL.

@lauriii Yeah i just started off contributing to drupal, will surely look into it.

@lauriii Made changes as per the comment #30.
Needs review.

Patch failed to apply. You can test a patch before uploading with the core/scripts/dev/commit-code-check.sh

So what patch was the correct one? I see there were a number of trial patches and I think #32 may have been off one of those. So should we review #25?

Sorry for the #32.
Made changes as per the comment #30.
Needs review.

Updated #28 Comment, Please review the patch

Added variables on component level. please review

Nesting looks good

But not sure if these spaces are needed

  --file-font-size: var(--font-size-s);

  min-block-size: calc(var(--file-space-m) + var(--file-offset));

  --file-size-color: var(--color-gray-800);

  color: var(--file-size-color);

But not sure if these spaces are needed

Even without spaces in pcss.css file, compiled CSS file have the space after declared variable. I think we need these space to differentiate between variables and properties.

If that's the case then this should be fine.

1. +++ b/core/themes/claro/css/components/file.pcss.css
   @@ -4,17 +4,21 @@
   +  --file-space-m: var(--space-m);
   +  --file-space-l: var(--space-l);
   

   These variables don't seem particularly helpful at least with the current naming 🤔 I think we need to either rename to something that explains what is their purpose in this component or just remove.

2. +++ b/core/themes/claro/css/components/file.pcss.css
   @@ -4,17 +4,21 @@
   +  --file-size-color: var(--color-gray-800);
   

   This should be added to the .file rule.

As per @lauriii's suggestion in #44, I have made changes to the .file rule.

Attaching the patch along with interdiff.

Kindly review. And please guide me if you find my changes inappropriate.

Thanks

Added updated patch and fixed CCF #46

Hiding patches #46 and #48

.file .file__size {
color: var(--file-size-color);
}

Is undoing some of the nesting previously done

And neither seem to address #44.1 so least do a restart back to #40 before continuing this forward.

So starting from #40 lets address the points in #44

// Ignore this patch
I am new to env. Checking the patch works or not.

Addressed #44, attached patch and interdiff with #40 for same. please review

#44 seems to be addressed.

Seems random failure.

I reviewed this Patch and doesn't seem to have any issue. The patch LGTM. But the error message that is mentioned typically appears in PHPUnit tests when an assertion is made that an object or array should be empty, but it is not.

Drupal\Tests\user\Kernel\Migrate\MigrateUserStubTest
fail: [Other] Line 0 of sites/default/files/simpletest/phpunit-1178.xml:
PHPUnit Test failed to complete; Error: PHPUnit 9.5.28 by Sebastian Bergmann and contributors.

Testing Drupal\Tests\user\Kernel\Migrate\MigrateUserStubTest
F                                                                   1 / 1 (100%)

Time: 00:01.502, Memory: 4.00 MB

There was 1 failure:

1) Drupal\Tests\user\Kernel\Migrate\MigrateUserStubTest::testStub
Failed asserting that an object is empty.

/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:122
/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:55
/var/www/html/core/modules/migrate_drupal/src/Tests/StubTestTrait.php:22
/var/www/html/core/modules/user/tests/src/Kernel/Migrate/MigrateUserStubTest.php:35
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:728

FAILURES!
Tests: 1, Assertions: 8, Failures: 1.

It is not related to the CSS code itself.

So i guess we can move it to RTBC.

Thanks

Random failure

Ticket isn’t having luck with random failures

Tested the patch #52 which is is showing fail testing,
It is applying cleanly.I think failure is not related to patch

Patch #52, still applies on 11.x. Restoring the status

Committed 9cd4453 and pushed to 11.x. Thanks!

This is a minor only change, removing tag for the followup.