[Discover] Hide the whole filter div instead of just the icons

[weltenwort]

Since #11604 the background of the filter icons in the document table remained visible and covered part of the cell content even when the cell was not hovered. That was caused by moving from display: none on the filter div to opacity: 0 on the individual icons.

This PR moves the background styles from the filter div to the individual icons to prevent it from covering cell content when not hovered.

fixes #12061

[cjcenizal]

Found an issue, but I think there's an easy solution.

[cjcenizal]

Unfortunately, if you tab to the buttons, they won't become visible because these styles only apply when the focus state is on the parent. I think the best solution is to remove the background-color: @panel-bg; property from line 43, and change .docTableRowFilterButton to have these styles:

/**
 * 1. Align icon with text in cell.
 * 2. Use opacity to make this element accessible to screen readers and keyboard.
 * 3. Show on focus to enable keyboard accessibility.
 */
.docTableRowFilterButton {
  appearance: none;
  background-color: @panel-bg;
  border: none;
  padding: 0 3px;
  font-size: 14px;
  line-height: 1; /* 1 */
  display: inline-block;
  opacity: 0; /* 2 */

  &:focus {
    opacity: 1; /* 3 */
  }
}

This way the buttons themselves have the background color, so they will only obscure the text when the buttons are focused or the row is being hovered over.

[weltenwort]

@cjcenizal thanks for the explanation, fixed it

[cjcenizal]

Nice! One small detail and then we're good.

[cjcenizal]

I would also add this rule:

  padding: 0 3px;

and remove this rule:

  & + & {
    margin-left: 5px;
  }

This way we can avoid a transparent gap between the buttons.

[lukasolson]

I merged this PR with your other one so I could see what the filter icons look like. This looks better, but when I hover over a cell in the highlighted row, the background of the icons clashes:

[weltenwort]

@lukasolson that's a good point

How about using a text shadow to create a contrast-increasing outline around the icons instead?

@cjcenizal what do you think of that?

[cjcenizal]

@weltenwort I like your idea but I think the bits and pieces of the cell text that show around the icons adds too much visual noise. I think we can solve this by updating the rule in discover/styles/main.less on line 135 to be:

    .discover-table-row--highlight {
      td,
      .docTableRowFilterButton {
        background-color: #E2F1F6;
      }
    }

Then the background color of the icons will match the highlighted background color:

[weltenwort]

Yes, that technically fixes it, but the coupling without an explicit block definition makes me cringe. It is merely a drop in an ocean of awful css in the doc table though, so here we go.

next up: doc table BEMification

@cjcenizal is there a high chance of conflict refactoring the doc table classes right now or would you say I'm safe to go ahead?

[cjcenizal]

makes me cringe

I'm glad I'm not the only one! :)

I think you're safe to refactor the doc table classes. There's nothing in the UI Framework that interacts with it, as far as I know. And I'm not working on any code that will affect it either.

You're a brave man @weltenwort! Viel glück.

[weltenwort]

@lukasolson @cjcenizal ready for another look

[cjcenizal]

LGTM! 🎸

[weltenwort]

@lukasolson do you have any further objections?

[lukasolson]

LGTM!