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

Adding help section for chat #5665

Merged
merged 1 commit into from Feb 26, 2015

Conversation

@ghost
Copy link

commented Feb 13, 2015

Problem with linking :/
final2

@jhass

This comment has been minimized.

Copy link
Member

commented Feb 13, 2015

The help section is clearly user oriented, we shouldn't include items for the podmin, in fact we shouldn't even display this section if the chat is disabled.

@ghost

This comment has been minimized.

Copy link
Author

commented Feb 13, 2015

Does client side has access to the configuration of the pod ?

@jhass

This comment has been minimized.

Copy link
Member

commented Feb 13, 2015

Not directly, that would be dangerous. I'm sure @Zauberstuhl knows a way to detect it from the client side though.

@Zauberstuhl

This comment has been minimized.

Copy link
Contributor

commented Feb 14, 2015

The first thing which pops into my mind is that you could check whether jsxc variable exists.
If you want a pure client side solution.

Probably the code is not loaded on the help page. Let me check that in the evening.

@ghost

This comment has been minimized.

Copy link
Author

commented Feb 14, 2015

If not, I can modify the help controller to pass whether tha chat is active or not in a data-section, for instance ?

@jhass

This comment has been minimized.

Copy link
Member

commented Feb 14, 2015

Yes, that would be the alternative if there's no general way of detecting it from the client side. You would use gon for that.

@ghost

This comment has been minimized.

Copy link
Author

commented Feb 14, 2015

How about the linking problem ? Seems like the HTML is escaped :/

@svbergerem

This comment has been minimized.

Copy link
Member

commented Feb 14, 2015

@AugierLe42e Your images include English text. We decided some time ago that we don't want to add images to the help section which would need a translation.

@ghost

This comment has been minimized.

Copy link
Author

commented Feb 14, 2015

@svbergerem : That's right. Let's see if I can include it in HTML.

@Zauberstuhl

This comment has been minimized.

Copy link
Contributor

commented Feb 14, 2015

Checked it and it is not possible. You should go with the controller

@ghost

This comment has been minimized.

Copy link
Author

commented Feb 14, 2015

Thank you.

@ghost

This comment has been minimized.

Copy link
Author

commented Feb 21, 2015

Works but still having troubles with Jasmine tests and link injections.

layout -> (c) { request.format == :mobile ? "application" : "with_header_with_footer" }

def faq
@data_chat = AppConfig.chat.enabled ? "enabled" : "disabled"

This comment has been minimized.

Copy link
@jhass

jhass Feb 21, 2015

Member

Let's use gon here.

This comment has been minimized.

Copy link
@ghost

ghost Feb 21, 2015

Author

Is it a usual use-case in d* ?
It seems a bit over-engineered to use a another just for that, right ?

This comment has been minimized.

Copy link
@jhass

jhass Feb 21, 2015

Member

It's already available and solves the problem better than your current solution. I don't see why that would be over-engineered, in fact it's less code you have to add in this PR.

This comment has been minimized.

Copy link
@ghost

ghost Feb 21, 2015

Author

in fact it's less code you have to add in this PR.

In fact it's more code I have to remove and change. But if you think it's better, then let's do this.

This comment has been minimized.

Copy link
@jhass

jhass Feb 21, 2015

Member

The changeset counts, not your work towards it ;P

@jhass

This comment has been minimized.

Copy link
Member

commented Feb 21, 2015

Could also use a rebase.

@@ -134,4 +141,28 @@ describe("app.views.Help", function(){
});
});
});

describe("chat section", function(){

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Feb 21, 2015

Expected 'describe' to have an indentation at 3 instead at 1.

This comment has been minimized.

Copy link
@ghost

ghost Feb 21, 2015

Author

You are right, milou !

},

chatEnabled: function(){
return gon.chat_enabled === 'enabled';

This comment has been minimized.

Copy link
@ghost

ghost Feb 21, 2015

Author

This doesn't work at all.

layout -> (c) { request.format == :mobile ? "application" : "with_header_with_footer" }

def faq
gon.preloads[:chat_enabled] = AppConfig.chat.enabled ? 'enabled' : 'disabled'

This comment has been minimized.

Copy link
@jhass

jhass Feb 21, 2015

Member

Here you use the key gon.preloads[:chat_enabled] but in the JS you use the key gon.chat_enabled. I find the key gon.chat_enabled better, so change this instance.

This comment has been minimized.

Copy link
@jhass

jhass Feb 21, 2015

Member

Also passing true/false to to it as returned by AppConfig.chat.enabled? should be just fine.

layout -> (c) { request.format == :mobile ? "application" : "with_header_with_footer" }

def faq
gon.chat_enabled = AppConfig.chat.enabled?

This comment has been minimized.

Copy link
@ghost

ghost Feb 21, 2015

Author

Doesn't work either. JS variable is undefined which make me think the variable is not corretly passed to the JS script.

This comment has been minimized.

Copy link
@jhass

jhass Feb 21, 2015

Member

Looks like gon converts that to headlessCamlCase.

This comment has been minimized.

Copy link
@ghost

ghost Feb 21, 2015

Author

Weird... Thanks for the tip. Works no. What about the tags not being injected correctly ?

injection

This comment has been minimized.

Copy link
@jhass

jhass Feb 21, 2015

Member

Handlebars feature. {{}} escapes, {{{}}} does not.

@ghost

This comment has been minimized.

Copy link
Author

commented Feb 21, 2015

},

linkImage: function(src, alt){
return '<div class="help-image"><img src="images/' + src + '" alt="' + alt + '"/></div>';

This comment has been minimized.

Copy link
@jhass

jhass Feb 21, 2015

Member

We got js_image_paths, use it here.

This comment has been minimized.

Copy link
@ghost

ghost Feb 21, 2015

Author

Is there something special to do for an image to be taken in account ?
Because :
injection
and though :
injection2

This comment has been minimized.

Copy link
@jhass

jhass Feb 22, 2015

Member

Does public/assets/ exist? Try removing it. Also try bin/rake tmp:cache:clear and restarting the app server.

This comment has been minimized.

Copy link
@ghost

ghost Feb 22, 2015

Author

Yay !
help

Just need to fix the jasmin test and we're done.

This comment has been minimized.

Copy link
@svbergerem

svbergerem Feb 22, 2015

Member

In most cases (when there are less than 20 contacts in the aspect) there will be also the mail icon in the control bar. The really important thing is the chat icon. I am still not convinced that we should add an image here. Why don't we describe where you will find the button (Go to the contacts page, select an aspect and click on the chat icon in the icon bar under the header to enable chat for the aspect) and add the original entypo icon?

@ghost

This comment has been minimized.

Copy link
Author

commented Feb 22, 2015

@svbergerem : Why do you want to make me do complicated things where an image is sufficient ? I followed your advice : I dropped the images with english text and I added your description. I don't think this is that important, really.

@svbergerem

This comment has been minimized.

Copy link
Member

commented Feb 22, 2015

@AugierLe42e You added a screenshot with 3 icons. As I already said in most cases there will also another icon (mail) next to those. We might add/remove/change icons in the future and then we will have to change the screenshot. The 3 icons from the screenshot are already in the browser's cache.

Instead of loading them another time as an image I suggested to add them as an icon. I think it would be sufficient if we would only include the chat icon but we could also display all 3 or 4 icons. For the code it doesn't really make a difference.

Instead of adding an image
<div class="help-image"><img src="' + ImagePaths.get(src) + '" alt="' + alt + '"/></div>
you would have to add an icon
<i class="entypo chat"></i>.
Why is that more complicated?

Adding those icons via entypo icons instead of an image makes it also easier to change the appearance of the icon later. We want to make the icon red? Just add color: red. We want to change the icon size? Just change font-size. If we would like to do the same with the screenshot we would either have to scale or edit the image. Both won't be as efficient.

@ghost

This comment has been minimized.

Copy link
Author

commented Feb 22, 2015

You may be right.

},

getChatIcons: function(){
return '<div class="help-chat-icons"> \

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Feb 23, 2015

Bad escaping of EOL. Use option multistr if needed.
Missing semicolon.


getChatIcons: function(){
return '<div class="help-chat-icons"> \
<i class="entypo lock-open"></i> \

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Feb 23, 2015

Bad escaping of EOL. Use option multistr if needed.

getChatIcons: function(){
return '<div class="help-chat-icons"> \
<i class="entypo lock-open"></i> \
<i class="entypo chat"></i> \

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Feb 23, 2015

Bad escaping of EOL. Use option multistr if needed.

return '<div class="help-chat-icons"> \
<i class="entypo lock-open"></i> \
<i class="entypo chat"></i> \
<i class="entypo trash"></i> \

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Feb 23, 2015

Bad escaping of EOL. Use option multistr if needed.

<i class="entypo lock-open"></i> \
<i class="entypo chat"></i> \
<i class="entypo trash"></i> \
</div>'

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Feb 23, 2015

Expected '

' to have an indentation at 5 instead at 1.
Expected an assignment or function call and instead saw an expression.
Missing semicolon.

This comment has been minimized.

Copy link
@ghost

ghost Feb 23, 2015

Author

Oh dear...

@ghost

This comment has been minimized.

Copy link
Author

commented Feb 23, 2015

@svbergerem : done. Will you help me to solve Jasmine test ? I think I messed again.

});

it('should display the chat', function(){
expect(this.view.$el.find('div[data-section=chat]').length).toBe(1);

This comment has been minimized.

Copy link
@svbergerem

svbergerem Feb 23, 2015

Member

use a[data-section=chat] instead.

beforeEach(function(){
gon.chatEnabled = true;
<<<<<<< HEAD
=======

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Feb 24, 2015

Bad line breaking before '==='.
Expected an identifier and instead saw '==='.
Unrecoverable syntax error. (79% scanned).

@ghost

This comment has been minimized.

Copy link
Author

commented Feb 24, 2015

Ok now ?

title: "Chat"
contacts_page: "contacts"
add_contact_roster_q: "How do I add contacts to my roster ?"
add_contact_roster_a: "Go to the %{contacts_page} page, select an aspect and click on the chat icon to enable chat for the aspect. %{toggle_privilege} Or when you create a new aspect we will also ask you if you would like to enable chat for that aspect"

This comment has been minimized.

Copy link
@svbergerem

svbergerem Feb 24, 2015

Member

I think this should be 'Go to the %{contacts_page}, select ...' and contacts_page should be 'contacts page'. Otherwise there might be languages where the current solution doesn't work.

@svbergerem

This comment has been minimized.

Copy link
Member

commented Feb 24, 2015

Apart from the comment I just added this looks good. I have no time right now to test this PR and merge it afterwards so either you will have to wait a few days or someone else merges this PR after you fixed the final remark.

chat:
title: "Chat"
contacts_page: "contacts page"
add_contact_roster_q: "How do I add contacts to my roster ?"

This comment has been minimized.

Copy link
@jhass

jhass Feb 24, 2015

Member

No Plenken please.

title: "Chat"
contacts_page: "contacts page"
add_contact_roster_q: "How do I add contacts to my roster ?"
add_contact_roster_a: "Go to the %{contacts_page}, select an aspect and click on the chat icon to enable chat for the aspect. %{toggle_privilege} Or when you create a new aspect we will also ask you if you would like to enable chat for that aspect"

This comment has been minimized.

Copy link
@jhass

jhass Feb 24, 2015

Member

Also missing period at the end. How about "for this aspect", or "make contacts of this aspect in the chat available"? I'd drop the "Or " from the next sentence and say "you can choose to" instead of "we will ask you".

@ghost

This comment has been minimized.

Copy link
Author

commented Feb 24, 2015

Corrected.

title: "Chat"
contacts_page: "contacts page"
add_contact_roster_q: "How do I add contacts to my roster?"
add_contact_roster_a: "Go to the %{contacts_page}, select an aspect and click on the chat icon to enable chat for the aspect. %{toggle_privilege} When you create a new aspect you can choose to enable chat for that aspect"

This comment has been minimized.

Copy link
@jhass

jhass Feb 24, 2015

Member

Still missing period for the second sentence. Looking at it again, the second "the aspect" in the first sentence seems a bit redundant maybe just replace it with "it". Also maybe the second sentence might better end in something like "for it too."

This comment has been minimized.

Copy link
@ghost

ghost Feb 24, 2015

Author

Maybe :

When you create a new one you can choose to enable chat for it ?

This comment has been minimized.

Copy link
@jhass

jhass Feb 24, 2015

Member

Guess we need @goobertron's help here ;)

This comment has been minimized.

Copy link
@goobertron

goobertron Feb 25, 2015

Hi! I'm here.

Is it just the one question and answer being added, or is there more text elsewhere? This one seems to cover only how to enable chat for an entire aspect, not how to actually add an individual contact to your chat roster. Will there be, or should there be, help available for other aspects of the chat feature?

Is the list of people in your chat interface officially called a 'roster', or is that your term, Augier? If the term doesn't appear in the chat interface, I'll probably choose another one as 'roster' isn't the most common term.

This comment has been minimized.

Copy link
@ghost

ghost Feb 25, 2015

Author

Will there be, or should there be, help available for other aspects of the chat feature?

Theorically, yes. The first that come to my mind is how to access the chat feature from an external client. And then, maybe, how to use video/audio.

Is the list of people in your chat interface officially called a 'roster', or is that your term, Augier?

Nope, this is the official JSXC terminology and this the term that appears on the wiki. I may not be the best term, though.

The question may not be the right one, actually. What about : "How do I fill my friends list ?"

This comment has been minimized.

Copy link
@Flaburgan

Flaburgan Feb 25, 2015

Member

No need to add the contact to the roster, once you activated the chat for an aspect, every contacts in this aspect are immediately available to chat with.

This comment has been minimized.

Copy link
@ghost

ghost Feb 25, 2015

Author

Finally, click their name in the chat interface and start a chat!

I think they'll that point by themselves ^_^

So, I'll propose :

How do I chat with someone in diaspora*?

You need to enable chat for one of the aspects that person is in. To do so, go to the contacts page, select an aspect and click on the chat icon to enable chat for the aspect. [Here, the icons] Or you can create a new and choose to enable chat for it by enabling the option [here, get the text displayed in the Facebox]

What do you think about it ?

This comment has been minimized.

Copy link
@goobertron

goobertron Feb 25, 2015

We're nearly there! How about:

How do I chat with someone in diaspora*?

First, you need to enable chat for one of the aspects that person is in. To do so, go to the %{contacts_page}, select the aspect you want and click on the chat icon to enable chat for the aspect. You could, if you prefer, create a special aspect called 'Chat' and add the people you want to chat with to that aspect. Once you've done this, open the chat interface and select the person you want to chat with.

I don't think any of the rest of the in-app help section contains images, so I wouldn't recommend adding them here. However, it would also be good to add a section about chat to the tutorials on the project site. To do that, make a PR in github. I'll work with you on that for the English, if you like.

This comment has been minimized.

Copy link
@ghost

ghost Feb 25, 2015

Author

It's not images it's just entypo icons ;)
Ok to me, I update the text.

This comment has been minimized.

Copy link
@goobertron

goobertron Feb 25, 2015

You forgot to update the question as well as the answer...

@ghost

This comment has been minimized.

Copy link
Author

commented Feb 25, 2015

@Zauberstuhl : whil working on this, maybe you want to add something ? Like how to connect with an external client ?

@Zauberstuhl

This comment has been minimized.

Copy link
Contributor

commented Feb 25, 2015

we can add items if it is merged. I have no access to your branch :)

chat:
title: "Chat"
contacts_page: "contacts page"
add_contact_roster_q: "How do I add contacts to my roster?"

This comment has been minimized.

Copy link
@goobertron

goobertron Feb 25, 2015

Should be "How do I chat with someone in diaspora*?"

font-size: 50px;
line-height: 70px;

i.entypo{

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Feb 25, 2015

Nesting should be no greater than 4, but was 5
Avoid qualifying class selectors with an element.
Selector should have depth of applicability no greater than 2, but was 5
Opening curly brace { should be preceded by one space

@@ -91,6 +91,18 @@ ul#help_nav {
border-radius: 0px 0px 4px 4px;
background-color: white;
padding: 10px 20px;

div.help-chat-icons{
text-align: center;

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Feb 25, 2015

Properties should be ordered font-size, line-height, text-align

@@ -91,6 +91,18 @@ ul#help_nav {
border-radius: 0px 0px 4px 4px;
background-color: white;
padding: 10px 20px;

div.help-chat-icons{

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Feb 25, 2015

Avoid qualifying class selectors with an element.
Selector should have depth of applicability no greater than 2, but was 4
Opening curly brace { should be preceded by one space

i.entypo{
color: #bfbfbf;

&.chat{ color: #000000; }

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Feb 25, 2015

Selector should have depth of applicability no greater than 2, but was 5
Opening curly brace { should be preceded by one space

layout ->(c) { request.format == :mobile ? "application" : "with_header_with_footer" }
end
before_filter -> { @css_framework = :bootstrap }
layout -> (c) { request.format == :mobile ? "application" : "with_header_with_footer" }

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Feb 25, 2015

Unused block argument - c. If it's necessary, use _ or _c as an argument name to indicate that it won't be used. Also consider using a proc without arguments instead of a lambda if you want it to accept any arguments but don't care about them.
Line is too long. [89/80]

def faq
gon.chatEnabled = AppConfig.chat.enabled?
end
end

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Feb 25, 2015

Final newline missing.

This comment has been minimized.

Copy link
@ghost

ghost Feb 25, 2015

Author

You are not very kind to me, Milou... Always criticizing me :',(

@ghost

This comment has been minimized.

Copy link
Author

commented Feb 26, 2015

Looks ok to me now.

@jhass

This comment has been minimized.

Copy link
Member

commented Feb 26, 2015

Please squash commits. Tipp: You can edit a commit with git commit --amend

this.CHAT_SUBS = {
add_contact_roster_a: {
toggle_privilege: this.getChatIcons(),
contacts_page: this.linkHtml('/contacts', Diaspora.I18n.t('chat.contacts_page'))

This comment has been minimized.

Copy link
@jhass

jhass Feb 26, 2015

Member

Oh, actually we wanted to start using js-routes for these.

@jhass

This comment has been minimized.

Copy link
Member

commented Feb 26, 2015

Thanks!

@jhass jhass added this to the next-major milestone Feb 26, 2015

@jhass jhass merged commit 168adae into diaspora:develop Feb 26, 2015

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
hound Hound has reviewed the changes.

jhass added a commit that referenced this pull request Feb 26, 2015

@ghost ghost deleted the chat-help-section-bis-repetita branch Mar 11, 2015

@ghost ghost restored the chat-help-section-bis-repetita branch Mar 11, 2015

@ghost ghost deleted the chat-help-section-bis-repetita branch Mar 11, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.