Problem/Motivation

Claro uses the default box-model "content-box", what causes unnecessary calculations for width and height (+ min-* & max-*) attributes.

Proposed resolution

Use "border-box" as described here: https://css-tricks.com/inheriting-box-sizing-probably-slightly-better-be...

I think border-box still is the de facto standard / best-practise since years, so i don't know why content-box is used in Claro?

CommentFileSizeAuthor
#13 3192130-13.patch1.18 KBswatidhurandhar
#10 3192130-10.patch1.04 KBgauravvvv

Issue fork drupal-3192130

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:

About issue forks

Comments

thomas.frobieter created an issue. See original summary.

Pooja Ganjage’s picture

Pooja Ganjage commented

Hi thomas.frobieter,

I don't see content-box defined anywhere in claro theme.

Would you please help me out for this.

Regards Pooja.

thomas.frobieter’s picture

thomas.frobieter
Primary language German
Location EU
commented

Hi @pooja-ganjage,

you're right, 'content-box' is the browsers default value, Claro MAY need to add 'border-box' as its default.

Maybe I'm wrong, but I think it's worth to discuss. Inside my "webdesign-bubble" it's a default thing, I don't see any benefit using content-box, because you often need to subtract the padding (& border-width) from the width & height.

Pooja Ganjage’s picture

Pooja Ganjage commented

Hi @thomas.frobieter,

I was go through the Claro theme and it has already defined whenever it's needed 'border-box'.

idebr’s picture

idebr commented
Component: CSS » Claro theme

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

Drupal 8 is end-of-life as of November 17, 2021. There will not be further changes made to Drupal 8. Bugfixes are now made to the 9.3.x and higher branches only. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.15 was released on June 1st, 2022 and is the final full bugfix release for the Drupal 9.3.x series. Drupal 9.3.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.4.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.9 was released on December 7, 2022 and is the final full bugfix release for the Drupal 9.4.x series. Drupal 9.4.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.5.x-dev branch from now on, and new development or disruptive changes should be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

gauravvvv’s picture

gauravvvv commented
Status: Active » Needs review
StatusFileSize
new 3192130-10.patch1.04 KB

I have attached the patch, please review

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Changes seems simple enough.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 10: 3192130-10.patch, failed testing. View results

swatidhurandhar’s picture

swatidhurandhar commented
Status: Needs work » Needs review
StatusFileSize
new 3192130-13.patch1.18 KB

I have created a new patch since #10 was failing through some test cases.

viren18febs’s picture

viren18febs commented
Status: Needs review » Reviewed & tested by the community
smustgrave’s picture

smustgrave commented
Version: 9.5.x-dev » 11.x-dev

11.x is the current development branch and #10 still applies

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 13: 3192130-13.patch, failed testing. View results

gauravvvv’s picture

gauravvvv commented
Status: Needs work » Reviewed & tested by the community

Random failure, restoring status

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Reviewed & tested by the community » Needs work

The Needs Review Queue Bot tested this issue.

While you are making the above changes, we recommend that you convert this patch to a merge request. Merge requests are preferred over patches. Be sure to hide the old patch files as well. (Converting an issue to a merge request without other contributions to the issue will not receive credit.)

ameymudras made their first commit to this issue’s fork.

ameymudras opened merge request !5370

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Needs work » Reviewed & tested by the community

restoring status, credited for target version update

nod_’s picture

nod_
Primary language French
Location Lille
commented

I do not understand what this changes practically.

The IS mentions that it causes unnecessary calculations but there is no simplifications in the code at all. I'm tempted to close this issue as "works as designed" if we don't have any other change in this MR.

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Reviewed & tested by the community » Needs work
smustgrave’s picture

smustgrave commented
Status: Needs work » Closed (works as designed)

Per #22 since there hasn't been any follow up.