Refactor Claro's system-admin--modules stylesheet

Gauravvv → made their first commit to this issue’s fork.

As the .css changes so much definitely think this needs before/after screenshots.

I have added before/patch screenshot.

Before

After

Thanks!

few pixel offset in LTR for the checkboxes and the description, new patch is more consistent between LTR and RTL so not an issue.

Question about unit mixing in a single rule on the MR.

Resolved the issue mentioned in #9

should have been more detailed in the feedback :p

Left a comment on MR @nod_

@Gauravvvv #13 you say you left a comment but don't see it?

working on it to address comment #12

Comment #12 addressed in this current patch.
Rem replaced with PX as suggest in the comment #12

This ticket was started in an MR and should be continued there. Especially since no interdiff was provided

@Gauravvvv what was the comment you mentioned?

Attached interdiff between last two commits,
Thanks

@smustgrave Pcss.css not generating (0 2px 4px rgba(0, 0, 0, 0.1) into rem after compilation. I tasted in my local the output was 0 2px 0.25rem rgba(0, 0, 0, 0.1);.
you can also refer the commit #b3156b80

In the commit #75ef2986 PX converted with rem and we can go with that.

@Gauravvvv what was the comment you mentioned?

let's use px everywhere. Postcss will do the conversion. This should be 0 2px 4px rgba(0, 0, 0, 0.1);

Earlier it was same --module-table-filter-box-shadow: 0 0.125rem 0.25rem rgba(0, 0, 0, 0.1); and postcss converts it into --module-table-filter-box-shadow: 0 2px 0.25rem rgba(0, 0, 0, 0.1);. That's why I have used rem to avoid this.

Reason behind this is, any value below 4px is not compiling to px from rem unit.

Sounds good to me.

@smustgrave your last comment #23 is response to #22 ? but you changed the status need review to need work if any change needed please mention so that we can work on this :)

I tested the #22 MR and working fine

Before

After

@smustgrove, you have updated the status to needs work and commented.

Sounds good to me.

Is the status update intentional or by mistake?

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Looks good!

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Working on this issue as a part of Claro Contribution Day.

I refactor the .pcss.css file. I was trying to rebase the current branch but facing some issues, so adding patch for now.
Here is the previous module list file.

Here are the screenshots of the changes.

We shouldn't revert back to the old way of doing things.

Issue might be the current MR is pointed at 10.1.x we need one for 11.x

I have updated leftover CSS logical properties. please review

Tested MR 5830 and it applied cleanly Now stylesheet using logical properties and nesting CSS

Attached after and before screenshots

Can we move this to RTBC now?

If you feel you've done a full code review, nothing has broken, and the core gates have been met then you can RTBC it https://www.drupal.org/about/core/policies/core-change-policies/core-gates →

Thanks @smustgrave for the guidance.

Moving this to RTBC LGTM

Mithun S → made their first commit to this issue’s fork.

Addressed the review comments and updated the PR. Changing the status to Needs review.

few details remaining

Addressed all the review comments and pushed a commit. Changing the status to Needs review.
Thank you!

appears feedback has been addressed

regresson on the "show all column" button when viewing on the mobile view.

i have implemented the solution as prescribed please review.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Seems to have a test failure, may need a rebase but should be tested.

@smustgrave, As far as I've tested nothing seems to be breaking! Attaching before and screenshits for reference.
Also seems like PhpUnit test is failing over here, but not sure why PhpUnit tests are failing when the MR only make chnages on PCSS and CSS files. Still trying to solve it by rebasing!

I've rebased from 11.x which seems to has resolved the pipeline. Ready for review

Left comments on MR.

Small comment.

sanket.tale → made their first commit to this issue’s fork.

Seemed to generate wrong see .css file

Feedback appears to be adderessed.

There is a couple of px of difference in RTL (3px difference on the checkbox position), not a big deal but would be nice to find why.

Couple of comments left.

I've given the same feedback twice in a couple of places. Please make sure feedback is kept when moving branches or trying to restart work somewhere.

smustgrave → changed the visibility of the branch 3332446-refactor-claros-system-admin--modules to hidden.

Small nitpicky comments on the comments.

I've actioned the above points (added back the comment) - see MR for an explanation of why I've placed it inside the selector.

PHPUnit Functional Javascript failures shown in the MR are not reproducible in local. Command used vendor/bin/phpunit --configuration phpunit.xml {filename}. Attaching screenshots as evidence. Please let me know if there is any other way to reproduce it in local. Moving this ticket back to Needs Work so that someone else can also verify it once.

stanzin → changed the visibility of the branch 3332446-refactor-claros-system-admin--modules-11x to hidden.