Remove background color from sidebar action

[juliushaertl]

The default action or menu should not have a background color set:

Before:

After:

[raimund-schluessler]

Fine with me, cypress regression base images need an update though. You can update them with npm run cypress:update-snapshots

[juliushaertl]

Fun, seems like spell checking caused a diff here.

[raimund-schluessler]

Fun, seems like spell checking caused a diff here.

@skjnldsv These are the things I was afraid of and proposed to run cypress inside a docker container also locally 🙈

[skjnldsv]

No, it was per Jan's request on the initial drafts to include this default background so it's easier to notice.

[skjnldsv]

Fun, seems like spell checking caused a diff here.

What does spell checking means here?
How could it cause a diff?

[raimund-schluessler]

The red wobbly line for wrong words:

[skjnldsv]

The red wobbly line for wrong words:

I'll be honest, that's funny :p

[raimund-schluessler]

So besides that the actual idea of this PR is not wanted by Jan, should we do something about that Cypress issue, i.e. move local cypress to docker? 🙈

[skjnldsv]

move local cypress to docker

Again, I'd like to avoid this, we'll lose all flexibility from the official cypress github actions.

[skjnldsv]

https://docs.cypress.io/api/plugins/browser-launch-api.html#Changing-browser-preferences
https://github.com/electron/electron/blob/master/docs/api/browser-window.md#new-browserwindowoptions

// cypress/plugins/index.js
module.exports = (on, config) => {
  on('before:browser:launch', (browser, launchOptions) => {
    if (browser.family === 'chromium' && browser.name !== 'electron') {
      launchOptions.preferences.default['browser.enable_spellchecking'] = false
      return launchOptions
    }

    if (browser.family === 'firefox') {
      launchOptions.preferences['layout.spellcheckDefault'] = 0
      return launchOptions
    }

    if (browser.name === 'electron') {
      launchOptions.preferences.spellcheck = false
      return launchOptions
    }
  })
}

[raimund-schluessler]

#1363 should disable the spell checking for cypress. Please test.

[juliushaertl]

No, it was per Jan's request on the initial drafts to include this default background so it's easier to notice.

I think it gives the wrong hint since we usually use that style for hover/focus feedback. Especially when using it in the compact sidebar mode this looks odd to me:

Let's see what @jancborchardt thinks about that once he is back.

[skjnldsv]

I think it gives the wrong hint since we usually use that style for hover/focus feedback. Especially when using it in the compact sidebar mode this looks odd to me:

Compact shouldn't have the background. Only expanded.
Please open an issue and discuss this with Jan, I just don't want regressions :)

[juliushaertl]

Ok, then let's close this for now.

[raimund-schluessler]

Indeed, only if the AppSidebar has an image / background set, the menu has no background.

Compact with a figure:

Compact without a figure:

But at least this is now consistent with an expanded sidebar with no figure 🙈 :

Expanded without a figure:

[skjnldsv]

Then this is an issue.

[skjnldsv]

Expanded without a figure:

This is not the proper screenshot ?

[raimund-schluessler]

Expanded without a figure:

This is not the proper screenshot ?

What do you mean? This is the screenshot made by cypress for
– subtitle: null
– starred: false
– compact: false
– header: none
– secondary: button
– editable: false

[skjnldsv]

AH, right.
"Expanded" is not the term I would have used there. This is confusing 🙈

[raimund-schluessler]

Sorry, I just thought of "expanded" as the opposite of "compact". But we can call it "non-compact" or so 😉