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

Render user actions separately #588

Merged
merged 1 commit into from
Jan 24, 2016
Merged

Render user actions separately #588

merged 1 commit into from
Jan 24, 2016

Conversation

xPaw
Copy link
Contributor

@xPaw xPaw commented Jan 17, 2016

This fixes #577, #404.

Needs to be done before merging this:

@dgw
Copy link

dgw commented Jan 17, 2016

What I can see of the previews from my phone looks good. Perfect or not, this is a big step forward from how it looks now. 👍

@dgw
Copy link

dgw commented Jan 17, 2016

See #571, though. The user button elements were changed to a to allow copying the text in Firefox. The new tpl should use as too.

@dgw
Copy link

dgw commented Jan 17, 2016

Never mind, it moved to #574 and still needs review. But the change will probably be merged.

@xPaw
Copy link
Contributor Author

xPaw commented Jan 17, 2016

I've tweaked the action texts, and it now looks like this:

yoloswag_ shout-_google_chrome_2016-01-17_22-22-33

@xPaw
Copy link
Contributor Author

xPaw commented Jan 17, 2016

Also formatted /me actions and added "Notice" prefix to notices.

wowtest_ shout-_google_chrome_2016-01-17_22-45-14

@MaxLeiter
Copy link
Contributor

+1 looks awesome

color: #ccc;
display: none;
font-style: normal;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

@xPaw, what does that remove already?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As you can see in the msg template, <em class="type">{{type}}</em> was removed as it's not needed anymore, it was used to display the action.

@astorije
Copy link
Collaborator

@xPaw, I left you a bunch of minor comments inline. This looks pretty awesome, really!
I hope we can merge that soon :-)

@xPaw
Copy link
Contributor Author

xPaw commented Jan 20, 2016

I've added displaying kick/part/quit messages, which will fix #404. However for this to work, slate-irc needs to be updated to the latest version, so that the message is passed through correctly.

EDIT: #597

@xPaw
Copy link
Contributor Author

xPaw commented Jan 23, 2016

@astorije I've added font awesome icons, looks pretty cool.

chrome_2016-01-23_17-26-38

EDIT: Made topic changes green:
chrome_2016-01-23_17-29-55

@astorije
Copy link
Collaborator

EDIT: Made topic changes green:

Wheren't you against bold/italic/colors in topic line? I'm OK with anything at the moment really.

}
#chat .kick .from:before {
font-family: FontAwesome;
content: "\f00d";
Copy link
Collaborator

Choose a reason for hiding this comment

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

For /kick, I'm not too fond of this icon, usually used for closing something and sometimes for deleting (but less and less, I notice).
I think I'd prefer ban (https://fortawesome.github.io/Font-Awesome/icon/ban/). If really you don't like it, the same icon as leave would work as the consequences are the same (the user leaves the channel) although I still prefer ban to that.
Alternatives could be:

But these I would prefer to notify a warning within the app.
And if really we want to go esoteric, these could do:

Meh, ban still conveys the most meaning, IMO :-)

@astorije
Copy link
Collaborator

These do look pretty cool... I look forward to that! :-)

@xPaw
Copy link
Contributor Author

xPaw commented Jan 24, 2016

Changed icons as per feedback

chrome_2016-01-24_12-01-16

Wheren't you against bold/italic/colors in topic line? I'm OK with anything at the moment really.

You are right, I've removed green again. And I would argue it's not the best idea to make it a separate line too.

erming added a commit that referenced this pull request Jan 24, 2016
Render user actions separately
@erming erming merged commit 62672e4 into erming:master Jan 24, 2016
@xPaw xPaw deleted the user-actions branch January 24, 2016 15:13
@astorije astorije self-assigned this Jan 24, 2016
@astorije
Copy link
Collaborator

I don't think this should have been merged already, while the discussion was ongoing... but nevermind...

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

Successfully merging this pull request may close these issues.

Display actions (such as join/leave/nick change) differently
5 participants