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 CreditAttribution: Pooja Ganjage at Asentech LLC 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
CreditAttribution: thomas.frobieter 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 CreditAttribution: Pooja Ganjage at Asentech LLC 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 CreditAttribution: idebr at iO 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 CreditAttribution: gauravvvv at Axelerant for Drupal India Association commented
Status: Active » Needs review
StatusFileSize
new 3192130-10.patch1.04 KB

I have attached the patch, please review

smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo 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 CreditAttribution: swatidhurandhar at Material for Drupal India Association 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 CreditAttribution: viren18febs at Concinnity Media Technologies for Concinnity Media Technologies commented
Status: Needs review » Reviewed & tested by the community
smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo 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 CreditAttribution: gauravvvv at Axelerant for Drupal India Association commented
Status: Needs work » Reviewed & tested by the community

Random failure, restoring status

needs-review-queue-bot’s picture

needs-review-queue-bot CreditAttribution: needs-review-queue-bot as a volunteer 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
CreditAttribution: nod_ as a volunteer and at Eclipse Foundation 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
CreditAttribution: nod_ as a volunteer and at Eclipse Foundation 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
CreditAttribution: nod_ as a volunteer and at Eclipse Foundation commented
Status: Reviewed & tested by the community » Needs work
smustgrave’s picture

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

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