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

feat(TopBar): show description during call #11428

Merged
merged 2 commits into from
Feb 6, 2024

Conversation

DorraJaouad
Copy link
Contributor

@DorraJaouad DorraJaouad commented Jan 23, 2024

☑️ Resolves

🖌️ UI Checklist

🖼️ Screenshots / Screencasts

🏚️ Before 🏡 After
image image

🚧 Tasks

  • Design review

🏁 Checklist

@Antreesy
Copy link
Contributor

Was introduced here: e7f3388
So I'd ask @marcoambrosini , why it has been done back then?

@marcoambrosini
Copy link
Member

marcoambrosini commented Jan 23, 2024

So I'd ask @marcoambrosini , why it has been done back then?

It was done to minimize information that was not crucial during a call. Actually @jancborchardt back then you were in favor of reducing text in the call view. We didn't even have the name and avatar, and it took me quite some convincing effort to add it there :)
Also, as far as I can remember, no call view from other apps shows conversation or meeting description while in the call.

Copy link
Member

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

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

We didn't add it because we wanted to put information like "people in call", "call duration", etc. there, but they are now in another place, so we can show the description there now.

@marcoambrosini
Copy link
Member

marcoambrosini commented Jan 23, 2024

We didn't add it because we wanted to put information like "people in call", "call duration", etc.

Nope, that proposal came later on, avatar and name were there before call time and participant count were added.

Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

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

@DorraJaouad nice! Just one thing: It looks like the text doesn’t have enough contrast, since it’s color-text-maxcontrast which is intended to be used on a light background but it’s used on dark. Either:

  • Is is possible to invoke the dark-theme color-text-maxcontrast for the subline here?
  • Or we use color-main-text for the subline here too, when in a call

@marcoambrosini Whatever was discussed in the past, most recent consensus was to add the description in the subline, same as how it is in the chat and as per the issue: #10562

@nickvergessen
Copy link
Member

Is is possible to invoke the dark-theme color-text-maxcontrast for the subline here?

color: var(--color-text-lighter);

@DorraJaouad DorraJaouad force-pushed the feat/10562/conversation-description branch from c74c6f1 to 691da45 Compare January 23, 2024 11:09
Copy link
Contributor

@Antreesy Antreesy left a comment

Choose a reason for hiding this comment

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

Looks brighter (#999999) with the dark-theme locally applied:

(new -> old)
image
image

@DorraJaouad DorraJaouad enabled auto-merge January 23, 2024 12:49
@jancborchardt
Copy link
Member

Is is possible to invoke the dark-theme color-text-maxcontrast for the subline here?

color: var(--color-text-lighter);

This is a deprecated variable though. Only text colors we should use are color-main-text and color-text-maxcontrast as per design guidelines: https://docs.nextcloud.com/server/latest/developer_manual/design/foundations.html#text-color

I'd say for simplicity and to make sure the contrast works, let's use color-main-text for the subline here too.


@Antreesy yes, the contrast works with dark theme enabled because the color-text-maxcontrast changes shade slightly. This is why I suggested:

Is it possible to invoke the dark-theme color-text-maxcontrast for the subline here?

@Antreesy
Copy link
Contributor

This is a deprecated variable though

Then we should also migrate to color-text-maxcontrast in some places.
For the color-main-text for a description, I can't see, why not. Conversation name is bold, so would be distinguishable enough

@DorraJaouad DorraJaouad force-pushed the feat/10562/conversation-description branch from 691da45 to 52a50b1 Compare January 23, 2024 14:42
@DorraJaouad
Copy link
Contributor Author

let's use color-main-text for the subline here too.

It conveyed 'too much' contrast to me so I opted for the dark theme color-text-maxcontrast
image

But I set it to color-main-text as suggested

Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

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

Looks good according to screenshots, and thanks for the deprecated variable fixes! :)

Signed-off-by: DorraJaouad <dorra.jaoued7@gmail.com>
…ondary style)

Signed-off-by: DorraJaouad <dorra.jaoued7@gmail.com>
@DorraJaouad DorraJaouad force-pushed the feat/10562/conversation-description branch from 52a50b1 to d05e9a2 Compare February 5, 2024 15:49
@DorraJaouad
Copy link
Contributor Author

Rebased onto master

@DorraJaouad DorraJaouad merged commit cfedde1 into main Feb 6, 2024
36 checks passed
@DorraJaouad DorraJaouad deleted the feat/10562/conversation-description branch February 6, 2024 09:07
@marcoambrosini
Copy link
Member

Can we at least go back to fading things out after some mouse inactivity?

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.

Conversation description not shown during call
5 participants