Skip to content
This repository was archived by the owner on Jan 15, 2019. It is now read-only.

Issues 78-80, 25#81

Open
LeviPetty wants to merge 76 commits intomasterfrom
Field_Session
Open

Issues 78-80, 25#81
LeviPetty wants to merge 76 commits intomasterfrom
Field_Session

Conversation

@LeviPetty
Copy link
Copy Markdown

Changes:

  • Implemented custom CSS loading.
  • Implemented custom logo loading.
  • Implemented config for site name and short site name.

@jackrosenthal jackrosenthal self-requested a review June 6, 2018 00:37
@LeviPetty LeviPetty changed the title Field session Issues 78-80 Jun 6, 2018
Comment thread acmwebsite/templates/archives.xhtml Outdated

<head py:block="head" py:strip="True">
<title>Mines ACM - Mailing List Archives</title>
<title>${tg.config.get('short_site_name', 'not configured')} - Mailing List Archives</title>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Default value should be in config/app_config.py and have a default of "Mozzarella".

Comment thread acmwebsite/templates/contact.xhtml Outdated
<html py:extends="master.xhtml" py:strip="True">
<head py:block="head" py:strip="True">
<title>Mines ACM - Contact</title>
<title>${tg.config.get('short_site_name', 'not configured')} - Contact</title>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

same

Comment thread acmwebsite/templates/error.xhtml Outdated
<html py:extends="master.xhtml" py:strip="True">
<head py:block="head" py:strip="True">
<title>Mines ACM - Error</title>
<title>${tg.config.get('short_site_name', 'not configured')} - Error</title>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

same

Comment thread acmwebsite/templates/index.xhtml Outdated
<py:extends href="master.xhtml" />
<head py:block="head" py:strip="True">
<title>Mines ACM</title>
<title>${tg.config.get('short_site_name', 'not configured')}</title>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

same

Comment thread acmwebsite/templates/index.xhtml Outdated
<div class="col-md-5">
<div class="intro-block">
<h2>Mines ACM Student Chapter</h2>
<h2>${tg.config.get('site_name', 'not configured')}</h2>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

same

Comment thread acmwebsite/templates/index.xhtml Outdated
<div class="panel panel-default">
<div class="panel-heading">
<h3 class="panel-title">Join our ACM Chapter!</h3>
<h3 class="panel-title">Join our ${tg.config.get('site_name', 'not configured')}!</h3>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

same


<head py:block="head" py:strip="True">
<title>Mines ACM - Survey ${survey_id} Responses</title>
<title>${tg.config.get('short_site_name', 'not configured')} - Survey ${survey_id} Responses</title>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

same

Comment thread development.ini.sample Outdated

# Set site name and short site name
#site_name =
#short_site_name =
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

use a key like site.name and site.short_name.

It is not clear what the purpose of name vs short name is. Please document or just use one.

Also, document the behavior when only one is specified.

Comment thread development.ini.sample Outdated
meetings.icalendar.prodid = -//Mines ACM//web//EN

# Custom Assets Configuration
custom_assets.dir = /home/lpetty/mozzarella/assets
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

should be commented out by default, as this directory definately wont exist for anyone but you.

Comment thread README.rst Outdated
Static Assets
~~~~~~~~~~~~~

The location of site-specific assets for development can be configured in ``development.ini``:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The site configuration might not be named development.ini.

Comment thread acmwebsite/templates/master.xhtml Outdated
<div class="header-logo">
<a href="${tg.url('/')}">
<img src="${tg.url('/img/full.svg')}" alt="Mines ACM" class="acm-logo" />
<img src="${tg.url(tg.config.get('custom_assets.logo'))}" alt="Club Logo" class="club-logo" />
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What's the default when no logo was specified? Can the site name be inserted instead?

@JadElClemens
Copy link
Copy Markdown
Contributor

Should be up to par now

Comment thread development.ini.sample Outdated
meetings.icalendar.prodid = -//Mines ACM//web//EN

# Custom Assets Configuration
custom_assets.dir = /
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It's a VERY bad idea to set the root of the filesystem as the custom assets dir, I would not reccomend this in the sample development.ini.

Try visiting /etc/passwd

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just use site.custom_assets for this name

Comment thread development.ini.sample Outdated
debug = true

# Site name configuration
#site.name = CSM Mozzarella
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just "Mozzarella"

Comment thread development.ini.sample Outdated
# Custom Assets Configuration
custom_assets.dir = /
custom_assets.css = custom.css
custom_assets.logo = custom.png
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think it's a good idea to just have the paths be equivalent to the logos under public, rather than configuring the file name. This allows the sysadmin the guaruntee that the only overridden assets will have the same paths as the public directory.

So these keys should not be necessary.

Comment thread acmwebsite/templates/master.xhtml Outdated
<div class="header-logo">
<a href="${tg.url('/')}">
<img src="${tg.url('/img/full.svg')}" alt="Mines ACM" class="acm-logo" />
<img src="${tg.url(tg.config.get('custom_assets.logo', 'logo.jpg'))}" alt="${tg.config.get('site.name')}" class="club-logo" />
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

See below comments, this will need changed.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just use the CSS class "logo". Also make sure to change the css class if you adjust this. BOTH sides need adjusted.

@JadElClemens JadElClemens changed the title Issues 78-80 Issues 78-80, 25 Jun 17, 2018
@JadElClemens
Copy link
Copy Markdown
Contributor

Now includes changes per #25, minus access control for certain pages and a web-based editor

Copy link
Copy Markdown
Contributor

@jackrosenthal jackrosenthal left a comment

Choose a reason for hiding this comment

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

Overall looks good. The wiki controller needs converted to OO style Page and Pages controllers. I can do if you do not have time.

I will also pull and test soon ... On mobile now.

Comment thread acmwebsite/controllers/wiki.py Outdated

def _init_wiki_repo(self):
self.repo = init_repository(tg.config.get('wiki.repo'), True)
signature = Signature("Example McPersonson", "emailuser@emailsite.tld") #TODO: Replace me
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Explain?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Explain what - the TODO?

If so - I just mean that this Signature can be replaced if need be, though it is only used for the inital commit. Presumably page commits using the web editor (once implemented) will pull signature details from the logged in profile.

Comment thread acmwebsite/controllers/wiki.py Outdated
try:
repo_path = tg.config.get('wiki.repo')
if not repo_path:
tg.abort(400, "Wiki not enabled")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

404

Comment thread acmwebsite/controllers/wiki.py Outdated
from docutils.core import publish_parts

import os
import tg
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Import modules before from import

Comment thread acmwebsite/controllers/wiki.py Outdated
return dict(pagename=pagename, parts=document)

@expose('acmwebsite.templates.wiki_history')
def history(self, pagename):
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we do in OO-manner like user profile controller?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

See f30995d - I attempted to follow the style of the user controller but it still seems a little dirty to me. Let me know what you think.

I moved the repo check/initialization logic from WikiController to WikiPagesController, but I had to change _before to init as it seems WikiPagesController wasn't calling _before like WikiController did.

<div class="container">
<div class="header-logo">
<a href="${tg.url('/')}">
<img src="${tg.url('/img/full.svg')}" alt="Mines ACM" class="acm-logo" />
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Careful ... Was css class adjusted as well?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Changed acm-logo to logo in style.css

Comment thread acmwebsite/templates/wiki_history.xhtml Outdated
</py:def>


<h1 class="page-header">History: <a href="${tg.url('/wiki/%s' % page)}">$page</a></h1>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do not use oldstyle string formatting

Comment thread acmwebsite/templates/wiki_view.xhtml Outdated
<nav class="navbar navbar-default">
<ul class="nav navbar-nav">
<li class="nav-item">
<a class="nav-link" href="${tg.url('/wiki/history/%s' % pagename)}">History</a>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Old style string formatting

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

How would you recommend formatting these URLs instead? I tried just doing '/wiki/history/$pagename' put that seemed to mangle the URL pretty bad.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is now fixed - wiki pages now follow the example from other controllers of using '[url]/{}'.format(var)


.. FIXME this still resolves to a local url
.. _reStructuredText format: <http://docutils.sourceforge.net/rst.html>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this FIXME resolved?

@JadElClemens
Copy link
Copy Markdown
Contributor

Programming for field session actually ended this past Friday, but I'm willing to see this PR through.

@JadElClemens
Copy link
Copy Markdown
Contributor

JadElClemens commented Jun 21, 2018

Seems like page history is now at /wiki/[pagename]/history

@JadElClemens JadElClemens dismissed jackrosenthal’s stale review June 23, 2018 02:41

Requested changes implemented in Field_Session

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants