Problem/Motivation

This particular change is coming from the feedback that we've received from the patch submission that we've made on 9/23/2020.

  1. +++ b/core/themes/olivero/olivero.theme
@@ -0,0 +1,583 @@
+    $variables['attributes']['class'][] = 'form-type--boolean';

    This is in violation of BEM because .form-type block element doesn't exist.

CommentFileSizeAuthor
#9 3173018-9.patch9.48 KBkostyashupenko
#7 3173018-7.patch7.06 KBkostyashupenko
#5 3173018-5.patch4.99 KBPooja Ganjage
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

proeung created an issue. See original summary.

kostyashupenko’s picture

kostyashupenko
Primary language Russian
Location Omsk
CreditAttribution: kostyashupenko at Skilld commented
Assigned: Unassigned » kostyashupenko
kostyashupenko’s picture

kostyashupenko
Primary language Russian
Location Omsk
CreditAttribution: kostyashupenko at Skilld commented
Project: Olivero » Drupal core
Version: 8.x-1.x-dev » 10.0.x-dev
Component: Code » Olivero theme
Status: Active » Needs work
kostyashupenko’s picture

kostyashupenko
Primary language Russian
Location Omsk
CreditAttribution: kostyashupenko at Skilld commented
Version: 10.0.x-dev » 9.2.x-dev
Pooja Ganjage’s picture

Pooja Ganjage CreditAttribution: Pooja Ganjage at Asentech LLC commented
FileSize
3173018-5.patch4.99 KB

Hi,

I am creating a patch for this issue.

Please review the patch.

Let me know for any suggestions.

Thanks.

Pooja Ganjage’s picture

Pooja Ganjage CreditAttribution: Pooja Ganjage at Asentech LLC commented
Status: Needs work » Needs review
kostyashupenko’s picture

kostyashupenko
Primary language Russian
Location Omsk
CreditAttribution: kostyashupenko at Skilld commented
Assigned: kostyashupenko » Unassigned
FileSize
3173018-7.patch7.06 KB

Attaching the patch, replaced `form-type--boolean` by `form-type-boolean`

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
CreditAttribution: mherchel commented
Version: 9.2.x-dev » 9.1.x-dev
Priority: Normal » Minor
Status: Needs review » Needs work
Issue tags: +CSS 
  1. +++ b/core/themes/olivero/css/components/form-boolean.pcss.css
@@ -87,7 +87,7 @@ input[type="radio"] {
+.form-type-boolean {

    We have the same exact selector on line 54 and 91. We should use this as an opportunity to refactor this file a bit to consolidate these selectors. 

  2. +++ b/core/themes/olivero/css/components/form-boolean.pcss.css
@@ -101,10 +101,10 @@ input[type="radio"] {
+.form-boolean-group .form-type-boolean {

    Same here. Let's use nesting and consolidate the selectors.

In addition to the above, let's remove the /* stylelint-disable-next-line csstools/use-logical */ comment from line 57. This refers to a stylelint plugin that was installed within contrib, but is not available to core.

kostyashupenko’s picture

kostyashupenko
Primary language Russian
Location Omsk
CreditAttribution: kostyashupenko at Skilld commented
Status: Needs work » Needs review
FileSize
3173018-9.patch9.48 KB

Diff and interdiff is a mess here, so attaching only patch. It should solve comment above

Status: Needs review » Needs work

The last submitted patch, 9: 3173018-9.patch, failed testing. View results

kostyashupenko’s picture

kostyashupenko
Primary language Russian
Location Omsk
CreditAttribution: kostyashupenko at Skilld commented
Status: Needs work » Needs review
mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
CreditAttribution: mherchel commented
Status: Needs review » Reviewed & tested by the community

Latest patch looks perfect. Thanks!

  • lauriii committed 7835007 on 9.2.x 
    Issue #3173018 by kostyashupenko, Pooja Ganjage, proeung, mherchel: [...

  • lauriii committed 691236d on 9.1.x 
    Issue #3173018 by kostyashupenko, Pooja Ganjage, proeung, mherchel: [...
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Status: Reviewed & tested by the community » Fixed

Committed 7835007 and pushed to 9.1.x and 9.2.x. Thanks!

Status: Fixed » Closed (fixed)

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