Skip to content
This repository has been archived by the owner on Dec 16, 2022. It is now read-only.

Update chat template so no users-icon shown when a server or private conversation is selected. #623

Closed
wants to merge 1 commit into from

Conversation

MaxLeiter
Copy link
Contributor

Fixes #622, should I add a font-awesome settings icon even if it won't do anything at the moment?

@MaxLeiter MaxLeiter changed the title Update chat template so no user-icon shown when a server is selected. Update chat template so no users-icon shown when a server is selected. Jan 27, 2016
@xPaw
Copy link
Contributor

xPaw commented Jan 27, 2016

👍

should I add a font-awesome settings icon even if it won't do anything at the moment?

Nah. Channels eventually should have settings too.

@AlMcKinlay
Copy link
Contributor

Definitely don't add a settings icon yet.

However, yeah, I don't think you should set this up as if the settings is only going to be on the lobby. I would just show user on anything other than lobby or user.

@MaxLeiter
Copy link
Contributor Author

Thats how it works with his PR @YaManicKill

@astorije
Copy link
Collaborator

should I add a font-awesome settings icon even if it won't do anything at the moment?

Definitely not, or we'll have a lot of "Settings don't work" issues popping up :-)

This is an off-topic side note that we should keep for later (the user settings design doc I was mentioning in #604), but I'm concerned about UI/UX regarding having a Settings button at the bottom left of the app (for global settings) and on the top right of each channel (for channel settings). I have ideas about that and will share them.

{{!-- Insert settings icon --}}
{{else}}
<button class="rt"></button>
{{/equal}}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm very not fond of placeholders like this one for potential future code/UI elements that might not be located there. Could you instead check that the type is not lobby then display the button, without else statement please?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually, checking that the type is a channel is probably more appropriate to make sure it doesn't show up on lobbies and private messages. Anything else we should display or not display this?

Copy link
Contributor

Choose a reason for hiding this comment

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

You're right, changing it to {{#equal type "channel"}} will do the job.

@astorije
Copy link
Collaborator

By the way, thanks a lot for that!! :-)

@astorije astorije self-assigned this Jan 28, 2016
@astorije astorije added the bug label Jan 28, 2016
Changes check to 'if channel' instead of 'if lobby else'
@astorije
Copy link
Collaborator

astorije commented Feb 1, 2016

@MaxLeiter, just to confirm, is shout.templates.js up-to-date with your change on chat.tpl?

@MaxLeiter
Copy link
Contributor Author

I forgot to run it a second time; will update tonight and message in IRC when I do.

@MaxLeiter
Copy link
Contributor Author

@astorije looks like I did update the template; ready to pull :)

@astorije
Copy link
Collaborator

astorije commented Feb 1, 2016

👍

@MaxLeiter MaxLeiter changed the title Update chat template so no users-icon shown when a server is selected. Update chat template so no users-icon shown when a server or private conversation is selected. Feb 2, 2016
@MaxLeiter
Copy link
Contributor Author

Worth noting that checking if its a channel also solved the issue of a user icon showing in private conversations.

@astorije
Copy link
Collaborator

astorije commented Oct 6, 2016

For the record, this was ported to The Lounge in PR thelounge/thelounge#32, appearing for the first time in v1.0.2.
Thanks again @MaxLeiter 😃

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants