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

fix(App): switch toggle button to dark theme in call #11040

Merged
merged 1 commit into from
Dec 6, 2023

Conversation

Antreesy
Copy link
Contributor

@Antreesy Antreesy commented Nov 29, 2023

☑️ Resolves

  • Drop overwritten styles for AppNavigationToggle in call
  • change theme to dark specifically for the toggle button

🖌️ UI Checklist

🖼️ Screenshots / Screencasts

Light theme Dark theme
image image
image image
Screencast.from.04.12.2023.11.18.58.webm

🏁 Checklist

  • 🌏 Tested with Chrome, Firefox and Safari or should not be risky to browser differences

@Antreesy
Copy link
Contributor Author

/backport to stable28

src/App.vue Outdated
@@ -750,6 +750,7 @@ export default {
:deep(.app-navigation-toggle) {
/* Force white handle when inside a call */
color: #D8D8D8;
background-color: transparent !important;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not transparent in @nextcloud/vue now intentionally. Otherwise, it overlaps background content on the mobile view.

image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After clearing overwritten styles:

Screencast.from.30.11.2023.12.56.38.webm

Dark theme:

Screencast.from.30.11.2023.12.58.00.webm

Not sure if we need to apply dark-theme here either

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if we need to apply dark-theme here either

We manually change all buttons in the call view to dark theme because the call view is always dark. Having it white seems very bright and eye-catching. Why not always use a dark theme here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @marcoambrosini for a design change review

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also agree that we should keep the buttons dark themed in the call view. It should look no different from other buttons in terms of styling.

Copy link
Contributor Author

@Antreesy Antreesy Dec 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But other buttons are not dark-themed... they are transparent with semi-transparent hover effect, which won't fit for the toggle overlapping them 🤕
I could apply a dark-theme colors here, but it still stands out for me

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think Maksim is right, using dark theme is not the common style in call view so it looks peculiar from other buttons.

Or is there a reason to make it stand out intentionally ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But the light theme looks even more eye-catching on a dark background around other dark buttons.

I'm fine with some other colors, but the transparent button overlaps on the mobile view.

Signed-off-by: Maksim Sukharev <[email protected]>
@Antreesy Antreesy changed the title fix(App): increase toggle button style specificity fix(App): switch toggle button to dark theme in call Dec 4, 2023
@Antreesy
Copy link
Contributor Author

Antreesy commented Dec 5, 2023

As background in call could be changed to a solid color (ref #11005), I'd go with basic dark-theme styles for now and merge

Then we could adjust the button color to mach the theme color (something similar to --color-primary-light):
image
Palette for the reference, it's hard to distinguish dark-theme primary-light color from grey shades 👀

@Antreesy Antreesy merged commit 0cc7077 into main Dec 6, 2023
36 checks passed
@Antreesy Antreesy deleted the fix/noid/fix-toggle-color branch December 6, 2023 09:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants