Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug]: Menus blend in with other elements #2779

Open
2 tasks done
psybers opened this issue May 18, 2024 · 15 comments · May be fixed by #2815
Open
2 tasks done

[Bug]: Menus blend in with other elements #2779

psybers opened this issue May 18, 2024 · 15 comments · May be fixed by #2815
Labels
bug Something isn't working

Comments

@psybers
Copy link
Contributor

psybers commented May 18, 2024

Verified issue does not already exist?

  • I have searched and found no existing issue
  • I will be providing steps how to reproduce the bug (in most cases this will also mean uploading a demo budget file)

What happened?

The popup menus have no border, which means they tend to blend in with other elements behind them making them less obviously a menu:

image

Simply open any menu and you can observe it. This affects the dark theme, though technically the other themes also have the problem. But in those other themes, the shadow added to the menu is enough to make it have somewhat of a border.

image image

Where are you hosting Actual?

Docker

What browsers are you seeing the problem on?

Chrome, Desktop App (Electron)

Operating System

Mac OSX

@psybers psybers added the bug Something isn't working label May 18, 2024
@Tigatok
Copy link
Contributor

Tigatok commented May 20, 2024

I think its mostly due to the searchbar background color being very similar to the popup menu color in dark mode, less about the shadow. That being said, some sort of technique could be applied to differentiate better

@psybers
Copy link
Contributor Author

psybers commented May 20, 2024

There are other places where this can happen. I discovered it while working on a popup menu inside a modal.

The dark theme needs to do something around the menus to help give them a border between them and the things behind them.

@Tigatok
Copy link
Contributor

Tigatok commented May 20, 2024

It actually doesn't look too terrible with a white border around in dark mode. Maybe something to consider

@youngcw
Copy link
Contributor

youngcw commented May 20, 2024

Those tooltips in darkmode used to be a different color, but it was getting to be such a middle of the road color that it was really hard to have good contrast with non-white text. We should do something to fix that color being the exact same as may backgrounds, but imo its better than it used to.

VoltaicGRiD added a commit to VoltaicGRiD/actual that referenced this issue May 24, 2024
…ibility and accessibility (actualbudget#2779)

Changed:
- /packages/desktop-client/stc/style/style.ts
- /packages/desktop-client/src/components/tooltips.tsx

Updated to include a border:
- color: theme.menuItemBackgroundHover
  - Used this color since its clean and maches the overall theming regardless of color palette
- width: 2px
  - Not too thin, not too thick
- style: solid
  - So it actually displays
@VoltaicGRiD
Copy link

VoltaicGRiD commented May 24, 2024

@psybers Have a look at these screenshots I pulled from the electron app. Let me know what you think:

image
image

I'm not convinced on the color of the border in light-mode, but it is visible for sure in my opinion.
Alternatively, I found another color that is far more "vocal" but doesn't look quite as clean:

image

The first one uses menuItemBackgroundHover
The second one uses menuItemTextHeader

@psybers
Copy link
Contributor Author

psybers commented May 24, 2024

@VoltaicGRiD Dark mode looks good. Do we even need to change the light mode ones? I think they worked as is.

@youngcw
Copy link
Contributor

youngcw commented May 24, 2024

I feel like adding borders would feel out of place since none of the other pop ups use borders. I second that the light theme has no issues as is and should't be changed.

@VoltaicGRiD
Copy link

@youngcw I'm definitely not against adding borders. I am all for accessibility and I feel like it adds that without detracting from the overall appearance or making it look "ugly" as one might say. Since they use themed variables, I was simply trying to find one that I could just apply globally to the tooltip windows, so in this case, I don't think either is necessarily a bad choice, especially considering they are mostly just a CSS styling choice.

@psybers Not necessarily, I do like that it stands out more, but I don't think its a requirement for the light theme. Might not be bad to find a middle-ground. I'll play with some colors,

@VoltaicGRiD
Copy link

VoltaicGRiD commented May 24, 2024

Alright, added a section to the themes for each theme that I think, in my non-designer non-frontend experience looks and feels quite nice:

image
image
image

Definitely no need for them all to be identical to the background color. Especially on dark themes like Midnight.

And just to be clear, this applies to all "tooltip" type objects, including the notes on the budget page.

VoltaicGRiD added a commit to VoltaicGRiD/actual that referenced this issue May 25, 2024
…ibility and accessibility (actualbudget#2779)

Changed:
- /packages/desktop-client/stc/style/style.ts
- /packages/desktop-client/src/components/tooltips.tsx

Updated to include a border:
- color: theme.menuItemBackgroundHover
  - Used this color since its clean and maches the overall theming regardless of color palette
- width: 2px
  - Not too thin, not too thick
- style: solid
  - So it actually displays

Changed color of tooltip to be more "vocal" against the background on all themes

Uses `theme.menuItemTextHeader` color now

Added whole style for 'tooltipXYZ' for each theme to add definition to colors (actualbudget#2779)
@psybers
Copy link
Contributor Author

psybers commented May 29, 2024

@VoltaicGRiD Making progress! I wonder if the color should match the border in between each row in the spreadsheet? It should still give a defined border, but would match the existing themes a bit more.

@VoltaicGRiD
Copy link

@VoltaicGRiD Making progress! I wonder if the color should match the border in between each row in the spreadsheet? It should still give a defined border, but would match the existing themes a bit more.

I don't hate it, but I think that they'd have to all match as themes, so
image
image

(Light doesn't change here, since it already fits that criteria)

If we just take the color from the table, I had to adjust the color of the background otherwise they were identical.

@psybers
Copy link
Contributor Author

psybers commented May 29, 2024

I don't know that these are perfect (to me), but they definitely feel closer than the prior screenshots for me. I think the issue with the prior ones was there was too much contrast between the border and the menu background. These, there is still contrast but it is more subtle.

With the prior ones, the border actually has more salience than the menu itself, which is probably backward. With the new versions that problem seems fixed.

@VoltaicGRiD
Copy link

I don't know that these are perfect (to me), but they definitely feel closer than the prior screenshots for me. I think the issue with the prior ones was there was too much contrast between the border and the menu background. These, there is still contrast but it is more subtle.

With the prior ones, the border actually has more salience than the menu itself, which is probably backward. With the new versions that problem seems fixed.

What a fantastic word

salience

I'll go ahead and commit these for now, I might add some custom shades that fit that 50% for me (gray750 instead of gray800). I think that'll be the solution that pleases everyone. Unless you had a better suggestion of course.

VoltaicGRiD added a commit to VoltaicGRiD/actual that referenced this issue May 30, 2024
…ibility and accessibility (actualbudget#2779)

Changed:
- /packages/desktop-client/stc/style/style.ts
- /packages/desktop-client/src/components/tooltips.tsx

Updated to include a border:
- color: theme.menuItemBackgroundHover
  - Used this color since its clean and maches the overall theming regardless of color palette
- width: 2px
  - Not too thin, not too thick
- style: solid
  - So it actually displays

Changed color of tooltip to be more "vocal" against the background on all themes

Uses `theme.menuItemTextHeader` color now

Added whole style for 'tooltipXYZ' for each theme to add definition to colors (actualbudget#2779)
@psybers
Copy link
Contributor Author

psybers commented May 30, 2024

@VoltaicGRiD If you have not already done so, I would recommend opening a pull request for these changes so we can work toward merging them later.

VoltaicGRiD added a commit to VoltaicGRiD/actual that referenced this issue May 31, 2024
…ibility and accessibility (actualbudget#2779)

Changed:
- /packages/desktop-client/stc/style/style.ts
- /packages/desktop-client/src/components/tooltips.tsx

Updated to include a border:
- color: theme.menuItemBackgroundHover
  - Used this color since its clean and maches the overall theming regardless of color palette
- width: 2px
  - Not too thin, not too thick
- style: solid
  - So it actually displays

Changed color of tooltip to be more "vocal" against the background on all themes

Uses `theme.menuItemTextHeader` color now

Added whole style for 'tooltipXYZ' for each theme to add definition to colors (actualbudget#2779)
VoltaicGRiD pushed a commit to VoltaicGRiD/actual that referenced this issue May 31, 2024
# This is the 1st commit message:

Avoid using underlines for emphasis (actualbudget#2765)

* Avoid using underlines for emphasis

* add release note
# This is the commit message actualbudget#2:

Updated tooltip / floating menu style to add a border for ease-of-visibility and accessibility (actualbudget#2779)

Changed:
- /packages/desktop-client/stc/style/style.ts
- /packages/desktop-client/src/components/tooltips.tsx

Updated to include a border:
- color: theme.menuItemBackgroundHover
  - Used this color since its clean and maches the overall theming regardless of color palette
- width: 2px
  - Not too thin, not too thick
- style: solid
  - So it actually displays

Changed color of tooltip to be more "vocal" against the background on all themes

Uses `theme.menuItemTextHeader` color now

Added whole style for 'tooltipXYZ' for each theme to add definition to colors (actualbudget#2779)
@MatissJanis
Copy link
Member

Related: #2815

@MatissJanis MatissJanis linked a pull request Jun 3, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants