az-digital / arizona-bootstrap

UArizona's front-end toolkit based on Bootstrap 4 and 5.
https://digital.arizona.edu/arizona-bootstrap
MIT License
5 stars 8 forks source link

Closes #1192: Add the CSS suggestions to avoid accordion titles overlapping arrows. #1198

Closed mmunro-ltrr closed 4 months ago

mmunro-ltrr commented 4 months ago

This only modifies the SCSS to generate the CSS suggestion from the issue. More testing is required to see if there are any side effects in various contexts.

danahertzberg commented 4 months ago

Need to add correcting CSS to collapsed accordions as well.

trackleft commented 4 months ago

Seems to work for me https://review.digital.arizona.edu/arizona-bootstrap/issue/1192/docs/2.0/components/collapse/#accordion-example

danahertzberg commented 4 months ago

I recommend this SCSS change to use flex display properties. Below is the whole file.

// Overrides & custom for the Collapse component
// Make entire accordion header clickable for accessibility and UX
.accordion {
  .card-header {
    padding: 0;
    h2,
    h3,
    h4,
    h5,
    h6,
    .h2,
    .h3,
    .h4,
    .h5,
    .h6 {
      padding: 0;
    }
    .btn {
      display: flex;
      width: 100%;
      padding: .75rem 1.25rem;
      color: #49595e;
      text-align: left;
      text-transform: none;
      justify-content: space-between;
      align-items: center;
      &::after {
        // stylelint-disable-next-line
        font-family: "Material Icons Sharp";
        font-size: 2em;
        -webkit-font-variant-ligatures: no-common-ligatures;
        font-variant-ligatures: no-common-ligatures;
        content: "expand_more";
        margin-left: 1rem;
        line-height: 1rem;
      }
    }
    .btn[aria-expanded="true"] {
      &::after {
        content: "expand_less";
      }
    }
  }
}
mmunro-ltrr commented 4 months ago

Here's the diff of the alternative suggestion from @danahertzberg

2,3c2
< 
< // make entire accordion header clickable for accessibility and UX
---
> // Make entire accordion header clickable for accessibility and UX
7d5
< 
20d17
< 
22c19
<       display: block;
---
>         display: flex;
25c22
<       color: $dark-silver;
---
>         color: #49595e;
28c25,26
< 
---
>         justify-content: space-between;
>         align-items: center;
30,33d27
<         position: absolute;
<         top: 2px;
<         right: 20px;
<         display: inline-block;
36a31
>           -webkit-font-variant-ligatures: no-common-ligatures;
38a34,35
>           margin-left: 1rem;
>           line-height: 1rem;
40d36
< 
danahertzberg commented 4 months ago

Here's the diff of the alternative suggestion from @danahertzberg

Thank you @mmunro-ltrr! Looks like the only follow up change I would make is to keep the color: $dark-silver; variable instead of changing it to the hex value. Should I make the change to this PR? Or would you like to?

danahertzberg commented 4 months ago

Not sure if something like this would work for the underline issue?

.accordion .card-header .btn-link:hover::after {
   text-decoration: none !important;
}
bberndt-uaz commented 4 months ago

Regarding the underline issue, see https://developer.mozilla.org/en-US/docs/Web/CSS/text-decoration:

Text decorations are drawn across descendant text elements. This means that if an element specifies a text decoration, then a child element can't remove the decoration.

It appears that normally the underline on ::after cannot be removed. One workaround is changing ::after to display: inline-block: however, in my testing, this only works if the button element is display: block and not display: flex. Bootstrap 5 uses display: flex but does not underline the accordion button text.

I would recommend either removing the underline entirely or reverting to the previous solution with the button as display: block.

mmunro-ltrr commented 4 months ago

Removing the underline entirely seemed the simplest solution, but unfortunately would affect many existing sites, since Arizona Barrio just applies the unmodified CSS from here to the Quickstart accordions.

danahertzberg commented 4 months ago

SO.. I can't remember how to test locally. BUT I may have a solution. What if we:

  1. Revert to my CSS in this PR
  2. Add the nested hover and span.title css below

    .btn {
      display: flex;
      width: 100%;
      padding: .75rem 1.25rem;
      color: $dark-silver;
      text-align: left;
      text-transform: none;
      justify-content: space-between;
      align-items: center;
    
      &:hover {
        text-decoration: none;
    
        span.title {
            text-decoration: underline;
          }
    
      }
  3. Add the <span class="title"> around {{ title }} in the file https://github.com/az-digital/az_quickstart/blob/main/modules/custom/az_accordion/templates/az-accordion.html.twig

Thoughts??

danahertzberg commented 4 months ago

Follow up issue to convert to flexbox https://github.com/az-digital/arizona-bootstrap/issues/1207

mmunro-ltrr commented 4 months ago

There's now a PR with a working version of the Flexbox approach, but with the downside that it requires HTML changes (see the documentation example). https://github.com/az-digital/arizona-bootstrap/pull/1208