Revert broken flexbox after Branding component creation.

Inotial tree before Branding component:

<div class="region region-header">
  <div class="block-system-branding-block">

so next styles worked:

.block-system-branding-block {
  flex: 0 1 40%;
}

after branding component creation tree became:

<div class="region region-header">
  <div class="block-system-branding-block">
    <div class="branding">

so this marked is `useless`:

.branding {
  flex: 0 1 40%;
}

Proposed resolution

1. Move flexbox child properties to higher level. EG: `block-system-branding-block`

OR

2. Rework branding component. But in this case according to BEM it shouldn't have any external geometry. And move that styles to `.region-header__branding`

CommentFileSizeAuthor
#16 Screenshot 2023-09-03 at 4.08.32 PM.png204.45 KBsmustgrave
#16 Screenshot 2023-09-03 at 4.08.32 PM.png204.45 KBsmustgrave
#7 interdiff-5_7.txt515 bytesgauravvvv
#7 3379522-7.patch753 bytesgauravvvv
#5 3379522-5.patch585 byteskostyashupenko

Issue fork drupal-3379522

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

Chi created an issue. See original summary.

chi’s picture

chi commented
Related issues: +#3379522: Revert broken flexbox after Branding component creation
nlisgo’s picture

nlisgo commented
Related issues: -#3379522: Revert broken flexbox after Branding component creation

@Chi I'm not sure if you intended to link to a different related issue but this is currently linking to itself.

chi’s picture

chi commented
Related issues: +#3365464: Create new SDC component for Umami Branding

Right. Added the correct one. Though I suspect those styles had become redundant before they were moved to SDC.

kostyashupenko’s picture

kostyashupenko
Primary language English
Location Omsk
commented
Status: Active » Needs review
StatusFileSize
new 3379522-5.patch585 bytes

I agree, so it's removed now. But i'm thinking globally about this - we are using BEM methodology, and BEM says that block-level element shouldn't have positioning styles. Normally speaking - branding component doesn't know where it will be displayed. In header, or in footer, or somewhere else. So normally except of removing `flex` property i also have to remove 

  .branding {
    margin: 2.5rem 0;
  }

And add this margin or for child elements of `.branding` bem-block, or from some parent component, like `.header .branding { margin: 2.5rem 0; }`

chi’s picture

chi commented

You are right. A component should not define external geometry.

But what about other CSS rules? Here is the content of that file.

/**
 * @file
 * This file is used to style the branding block.
 */

.branding {
  flex: 0 1 40%;
}

@media screen and (min-width: 48em) {
  .branding {
    flex: 0 1 220px;
    margin: 2.5rem 0;
    text-align: left;
  }
}

.branding__site-logo {
  display: inline-block;
  width: 100%;
  max-width: 205px;
  background-color: inherit;
}

.branding__site-logo:hover,
.branding__site-logo:focus {
  background-color: inherit;
}

.branding__site-logo svg {
  width: 100%;
  max-width: 205px;
  height: auto;
}

And the corresponding HTML.

<div class="branding">
  <a href="/umami/web/en" rel="home" class="branding__site-logo">
    <img src="/umami/web/core/profiles/demo_umami/themes/umami/logo.svg" alt="Home">
  </a>
</div>

Could you find a single line here that still makes sense, besides the margin?

Note that the component has no any text nodes. And the logo is rendered via img tag.

gauravvvv’s picture

gauravvvv commented
StatusFileSize
new 3379522-7.patch753 bytes
new interdiff-5_7.txt515 bytes

I have removed some unused styles, attached interdiff for same.

.branding__site-logo:hover,
.branding__site-logo:focus {
  background-color: inherit;
}

This code is required, If we eliminate the background, the background color appears on the anchor tag. This will also modify the background color of the logo image. 

a:hover, a:focus {
    color: #cc2a00;
    background-color: #e6eee0;
}
Harish1688’s picture

Harish1688 commented

Hi,

Tested the patch 3379522-7.patch from comment #7, and found unused styles in Umami branding component are removed successfully from the file without any UI impact on design.

Also tested the this code, it will also modify the background color of the logo image.

a:hover, a:focus {
    color: #cc2a00;
    background-color: #e6eee0;
}

Tested steps:
1. Drupal 11 setup with Umami theme.
2. applied the patch and verified the patch changes.

Looks Good for RTBC+

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Bug Smash Initiative

Per testing #8

chi’s picture

chi commented

Still think that the whole branding.css can be dropped.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Reviewed & tested by the community » Needs work

I think this is unused at the moment but this was added for a reason; to reduce the size of the logo on mobile. Looks like #3379522: Revert broken flexbox after Branding component creation caused a regression to the mobile styles. Maybe we should repurpose this issue to address that regression? It's also a good opportunity for us to figure out how we want to handle layouts with components.

chi’s picture

chi commented

Looks like #3379522: Unused styles in Umami branding component caused a regression to the mobile styles.

You referenced this issue. I guess it supposed to be some other issue.

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

finnsky opened merge request !4694

finnsky’s picture

finnsky commented
Status: Needs work » Needs review

FIxed position regression.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
StatusFileSize
new Screenshot 2023-09-03 at 4.08.32 PM.png204.45 KB
new Screenshot 2023-09-03 at 4.08.32 PM.png204.45 KB

Tested on mobile and see that the logo size changes

before

Only local images are allowed.

after

after

edit. wrong before picture.

finnsky’s picture

finnsky commented
Status: Needs work » Needs review

@smustgrave Yes! This is exactly what was fixed. It was broken while branding component was created and now fixed.

finnsky’s picture

finnsky commented

You can see styles on place now.
https://gyazo.com/2b9fea383718076ba29d7db871b9b9ba

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: +Needs issue summary update, +Needs title update

Issue summary and title should be updated if this is fixing a bug vs removing unused code please

finnsky’s picture

finnsky commented
Title: Unused styles in Umami branding component » Revert broken flexbox after Branding component creation
Issue summary: View changes
finnsky’s picture

finnsky commented
Issue summary: View changes
finnsky’s picture

finnsky commented
Status: Needs work » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs issue summary update, -Needs title update

Think this is ready for RTBC bucket.

  • lauriii committed 8e56fd31 on 11.x 
    Issue #3379522 by finnsky, Gauravvvv, kostyashupenko, smustgrave, Chi:...

  • lauriii committed c754c4c0 on 10.1.x 
    Issue #3379522 by finnsky, Gauravvvv, kostyashupenko, smustgrave, Chi:...
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Version: 11.x-dev » 10.1.x-dev
Status: Reviewed & tested by the community » Fixed

Committed 8e56fd3 and pushed to 11.x. Also cherry-picked to 10.1.x. Thanks!

lauriii closed merge request !4694

Status: Fixed » Closed (fixed)

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