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

Replace triangle sidebar trigger with dedicated icon, add icons to sidebar tabs and fix padding #548

Merged
merged 7 commits into from
Jan 9, 2018

Conversation

jancborchardt
Copy link
Member

Sidebar trigger icon:
screenshot from 2018-01-09 17-20-11

And in the sidebar:
screenshot from 2018-01-09 17-20-27

Please review @danxuliu @nickvergessen @Ivansss

Copy link
Member

@danxuliu danxuliu left a comment

Choose a reason for hiding this comment

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

I like a lot the icons in the tab headers :-)

Regarding the other one... I still prefer my triangle :-P But even with #546 the triangle will have some consistency problems regarding the highlight (as it would highlight by being more clear while other elements will be highlighted by being more strong), so... I think it is a :+1 ;-)

However, shouldn't the shadow be more subtle on a white background? I know that it makes it more noticeable, but I think it is too much (not too much noticeable, I mean too much shadow; being noticeable is good ;-) ).

@jancborchardt
Copy link
Member Author

@danxuliu Yeah, when we are still not in the call / on white background we could even just remove the icon-white icon-shadow classes from the icons. :)

Let’s do that in a separate pull request though. Is there a class which lets us know if we are in a call or not?

Signed-off-by: Jan-Christoph Borchardt <hey@jancborchardt.net>
Signed-off-by: Jan-Christoph Borchardt <hey@jancborchardt.net>
Signed-off-by: Jan-Christoph Borchardt <hey@jancborchardt.net>
Signed-off-by: Jan-Christoph Borchardt <hey@jancborchardt.net>
Signed-off-by: Jan-Christoph Borchardt <hey@jancborchardt.net>
Signed-off-by: Jan-Christoph Borchardt <hey@jancborchardt.net>
Signed-off-by: Jan-Christoph Borchardt <hey@jancborchardt.net>
@danxuliu
Copy link
Member

danxuliu commented Jan 9, 2018

Is there a class which lets us know if we are in a call or not?

The incall class is added to the #app-content element... in theory. I have just seen that sometimes it is not added. Great... :-P

@nickvergessen
Copy link
Member

Hmm not sure I like all these white icons on the white background (shadowed or not), I find them hard to see. Especially when the redshift kicks in. I'd say: either use grey/black like with all other icons, or stay with the triangle. (And yes, we can change the icon to white when you are switching into a call, so it's still visible on the black screen).

But 👍 for the icons on the tab headers.

@jancborchardt
Copy link
Member Author

@nickvergessen yep, that's why I mentioned that we can simply show them as grey icons when on white background. :) Will do that, but in a separate PR.

@jancborchardt
Copy link
Member Author

(Cause it's the same for the mute and video icons.)

@jancborchardt jancborchardt merged commit 51d2039 into master Jan 9, 2018
@jancborchardt jancborchardt deleted the icons branch January 9, 2018 20:57
@jancborchardt
Copy link
Member Author

The dark icons are in #553 :)

marcoambrosini pushed a commit that referenced this pull request Oct 9, 2019
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.

3 participants