Clean up the "Featured-top" component in Bartik

Investigated this a bit.

Currently featured is mentioned in 5 different css-files.

1. featured.css
2. media.css
3. messages.css
4. layout.css
5. admin.css

First four are pretty simple, but I'm not sure if the demo-block related css should be placed to it's own component-file or to featured.css.

On template we have following featured-related things:

    {% if page.featured %}
      <div id="featured">
        <aside class="section clearfix" role="complementary">
          {{ page.featured }}
        </aside> <!-- /.section -->
      </div> <!-- /#featured -->
   {% endif %}

I don't really understand the usage of aside on this kind of region and by what we've been talking on #2398471: Clean up the "footer" component in Bartik, i think that this should be probably done like:

    {% if page.featured %}
      <div class="site-featured__wrapper">
        <div class="site-featured clearfix" role="complementary">
          {{ page.featured }}
        </div>
      </div>
   {% endif %}

I think the

Is the duplicate .featured__wrapper/.featured wrapper used for anything or can it be removed?

There was different styles wrote for .featured_wrapper and .featured, but currently it looks like .featured styles are not actually doing anything. I'll rewrite it so that we don't have anything on .featured_wrapper and if everything goes well, I'll remove the it completely.

Started to re-do on this. Sending this patch to see what test-bot is saying. Took the wrapper out completely.

On this patch the markup for featured is

<div class="featured">
  <div class="region-featured">

Before it was

<div id="featured>
  <aside class="section">
    <div class="region-featured">

Region-featured stuff is actually coming somewhere out of the template. I also had to change the bartik.theme because it was putting class featured to body. Changed that to "has-featured".

1. +++ b/core/themes/bartik/css/components/featured.css
   @@ -12,11 +12,35 @@
   +@media all and (min-width: 520px) {
   +
   +  /* ----------------- Featured ----------------- */
   +  .featured {
   +    font-size: 1.643em;
   +  }
   +  .featured h2 {
   +    font-size: 1.174em;
   +  }
   +}
   

   Can we move this code to be directly below the elements it's editing? Also we can remove the comment within it

2. +++ b/core/themes/bartik/css/components/featured.css
   @@ -12,11 +12,35 @@
   +@media all and (min-width: 901px) {
   +  .has-featured .region-primary-menu .menu li a:active,
   +  .has-featured .region-primary-menu .menu li a.active {
   +    background: #f0f0f0;
   +    background: rgba(240, 240, 240, 1.0);
   +  }
   +}
   

   I actually this this styling should go with the primary-menu component, because .has-featured is a modifier of the menu component in this case. Also, this styling makes no sense, what does it do?

3. +++ b/core/themes/bartik/templates/page.html.twig
   @@ -117,10 +117,8 @@
   -      <div id="featured">
   -        <aside class="section clearfix" role="complementary">
   -          {{ page.featured }}
   -        </aside>
   +      <div class="featured">
   +        {{ page.featured }}
   

   We've lost the semantics of the aside element and the role attribute. Can we add them back in?

Are we missing a deleted file from the patch? I see some CSS added that isn't remove from somewhere else.

#6.1 Agreed
#6.2 With styles:

Without styles:

It is true that it doesn't make sense. Correct breakpoint is somewhere around 810px, but I decided to keep it this way and create follow up on this.
#6.3 Can be done, but I still dislike the idea of having minimum of 4 wrapper'ish things on this kind of region to output anything. Your suggestion?

Worked a bit on this, not needing a review on this patch.

After the region was renamed rerolled and added the semantics of aside back.

It seems that I forgot to add changes on bartik.theme to latest patch.

Visually, the patch in #16 looks perfect. That being said, it needs some work to conform to the Drupal 8 CSS standards (see https://www.drupal.org/node/1887918#sub-components)

So, ideally we should see something like

<div class="featured-top">
<aside class="featured-top__inner clearfix" role="complementary">

with a class name of

.featured-top__inner  {...}

I'm taking a look at this

Updated patch

Thanks for the fixes. Here is a screenshot to show everything looking lovely.

I'm not saying the __inner class if wrong, but we didn't put anything similar to footer, right?

And do we really want to use the

.wrapper
  .inner
     .region

structure?

I think we have something similar:

.footer is similar to featured__inner
.featured is similar to footer__wrapper

So, I don't know if we want to decide between using either __wrapper subcomponents or __inner subcomponents in Bartik or across of all themes. It feels a bit like a bikeshed discussion, shall we discuss this in a follow up?

Sorry! I spotted something

+++ b/core/themes/bartik/css/layout.css
@@ -10,7 +10,7 @@ body,
-#featured-top aside.section,
+.feature-top__inner,

I noticed that we have a small typo here.

I was a little confused about has-featured-top vs. featured-top but I see one goes in the body tag and the other goes on the div.

Committed and pushed to 8.0.x. Thanks!