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

Sahana Eden Chat #772

Merged
merged 1 commit into from May 29, 2014

Conversation

Projects
None yet
6 participants
@arnavkagrawal
Contributor

arnavkagrawal commented May 24, 2014

No description provided.

@@ -83,5 +84,31 @@
</div>
{{pass}}
{{include "scripts.html"}}
{{if settings.get_chat_status():}}

This comment has been minimized.

@nursix

nursix May 24, 2014

Member

Move into separate view template and include?

This comment has been minimized.

@flavour
@@ -11,6 +11,7 @@
../themes/default/style.css
../themes/default/homepage.css
../themes/default/mobile.css
../themes/default/converse.css

This comment has been minimized.

@nursix

nursix May 24, 2014

Member

That's a lot of CSS to include routinely even if chat is off

@@ -0,0 +1 @@
#crontab

This comment has been minimized.

@debugger22

debugger22 May 25, 2014

A newline here?

"""
If chat module is enabled or disabled
"""
return self.base.get("chat_enabled",False)

This comment has been minimized.

@debugger22

debugger22 May 25, 2014

A space after , according to PEP8.
"chat_enabled",False -> "chat_enabled", False

@arnavkagrawal

This comment has been minimized.

Contributor

arnavkagrawal commented May 25, 2014

@flavour please review :)

@@ -32,7 +32,9 @@
{{# Built by /static/scripts/tools/build.sahana.py }}
<link href="/{{=appname}}/static/themes/{{=theme}}/eden.min.css" rel="stylesheet" type="text/css" />
{{pass}}
<link href="/{{=appname}}/static/themes/default/converse.css" rel="stylesheet" type="text/css" />

This comment has been minimized.

@nursix

nursix May 25, 2014

Member

That's not really smarter than before - the stylesheet is fairly big, and if linked like this it's not even getting minified.

Couldn't it be made conditional? So that at least if chat is disabled it doesn't get downloaded unnecessarily? Maybe it's asking too much at this stage - but if it's included unconditionally, then it should be in the minify-config.

This comment has been minimized.

@flavour

flavour May 25, 2014

Member

I suggested moving this into the conditionally-included View template.
It can be made to load after page load into HEAD using JS (like is done for Ext's CSS)

This comment has been minimized.

@flavour

flavour May 25, 2014

Member

& yes, we should have debug & minified versions of the CSS

This comment has been minimized.

@arnavkagrawal

arnavkagrawal May 25, 2014

Contributor

I will look into how to do it like EXT's and rebase it :)

@@ -126,6 +126,12 @@ def get_template(self):
"""
return self.base.get("template", "default")
def get_chat_status(self):
"""
If chat module is enabled or disabled

This comment has been minimized.

@flavour

flavour May 25, 2014

Member

comment should include that the positive isn't True but rather the IP of the XMPP server

This comment has been minimized.

@arnavkagrawal

arnavkagrawal May 25, 2014

Contributor

will do it :)

This comment has been minimized.

@flavour

flavour May 25, 2014

Member

& therefore the name of the setting & API call should change:
settings.base.chat_server
settings.get_chat_server()

<div id="conversejs"></div>
<script>

This comment has been minimized.

@flavour

flavour May 25, 2014

Member

Would be better to have this in static (so it's minified, cached & not server-side processed)

This comment has been minimized.

@flavour

flavour May 25, 2014

Member

I would minify this with the main converse.js

auto_list_rooms: true,
auto_subscribe: false,
// bosh_service_url: 'http://127.0.0.1:7070/http-bind/', // Please use this connection manager only for testing purposes
bosh_service_url: document.getElementById("boshurl").innerHTML, // Please use this connection manager only for testing purposes

This comment has been minimized.

@flavour

flavour May 25, 2014

Member

This seems an odd way to pass the config. Instead I would suggest having it put into S3 global
So the view template would have:
s3.js_global.append('''S3.chat_url={{{settings.get_chat_server()}}}'''
& the static would have:
bosh_service_url: S3.chat_url

This comment has been minimized.

@ptressel

ptressel May 26, 2014

Contributor

Arnav and I are chatting -- we think we're getting closer. Arnav is just trying this:
{{s3.js_global.append(”'S3.chat_url=http://%s/http-bind”' % settings.get_chat_server())}}

@arnavkagrawal

This comment has been minimized.

Contributor

arnavkagrawal commented May 26, 2014

@flavour please review :)

s3 = current.response.s3
request = current.request
appname = request.application
chat_server = current.deployment_settings.get_chat_server()

This comment has been minimized.

@graeme-f

graeme-f May 26, 2014

Contributor

You don't use request, or appname so little point in declaring them
Likewise chat_server is only referenced once so it would be better to do away with that variable.
Similarly with s3, it's only used once.

// allow_otr: true,
auto_list_rooms: true,
auto_subscribe: false,
// bosh_service_url: 'http://127.0.0.1:7070/http-bind/', // Please use this connection manager only for testing purposes

This comment has been minimized.

@graeme-f

graeme-f May 26, 2014

Contributor

Is this still needed, it looks like a testing leftover?

@arnavkagrawal

This comment has been minimized.

Contributor

arnavkagrawal commented May 28, 2014

@flavour please review :)

@arnavkagrawal

This comment has been minimized.

Contributor

arnavkagrawal commented May 28, 2014

@flavour please review

@arnavkagrawal

This comment has been minimized.

Contributor

arnavkagrawal commented May 29, 2014

@flavour Please review.

@arnavkagrawal

This comment has been minimized.

Contributor

arnavkagrawal commented May 29, 2014

@flavour please review :)

  1. Set up the page http://eden.sahanafoundation.org/wiki/InstallationGuidelines/Chat. (there in the comments of 000_config.py. Will setup other pages and link them.
  2. Changed the filename from chat.js.cfg to sahana.chat.js.cfg

flavour added a commit that referenced this pull request May 29, 2014

@flavour flavour merged commit f217937 into sahana:master May 29, 2014

@arnavkagrawal arnavkagrawal deleted the arnavkagrawal:chat branch Jun 4, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment