consolidate/combine site_base and view_base templates #103

Closed
rlskoeser opened this Issue Aug 3, 2013 · 17 comments

Comments

Projects
None yet
4 participants

I'm not certain what the differences are, but at least we should extract all common elements into a single base template so simple fixes to site headers, footers, etc can be made once for all site pages.

Contributor

scottkleinman commented Aug 3, 2013

Yes, this was a bit of a hack on my part. I think the only real differences now are the size of the header logo. We might be able to address that server side or with a conditional in the template. Definitely something to look into.

I've started a feature branch to refactor this and I think I have it most of the way there, but I'm still seeing some discrepancies (and quite possibly there are other discrepancies I'm not seeing). I'm willing to do some more clean up on it myself first, but also willing to push the feature branch for others to look at and help. Will definitely need someone to vet the changes I've made and make sure nothing got lost or messed up.

Contributor

mialondon commented Aug 3, 2013

It'd be good to have @fontnerd and @amandavisconti check in on this as they have the most to do with style variations for different site sections.

Contributor

scottkleinman commented Aug 4, 2013

I'll try to look at it as well, since I did the original split into two
files.

On 3 August 2013 13:23, Mia notifications@github.com wrote:

It'd be good to have @fontnerd https://github.com/fontnerd and
@amandavisconti https://github.com/amandavisconti check in on this as
they have the most to do with style variations for different site sections.


Reply to this email directly or view it on GitHubhttps://github.com/chnm/serendipomatic/issues/103#issuecomment-22060976
.

Scott Kleinman
Professor of English
Director, Center for the Digital Humanities
California State University, Northridge

just pushed the branch with what I did so far - take a look at feature/template-refactor

tried to document which templates are used where

it's close but a few things are a bit off. wondered about taking html generated from current/new and sanitizing/formatting so we can compare and identify what's missing

Contributor

fontnerd commented Aug 7, 2013

I'm a bit lost on how I can help here. Not quite sure what I should be looking for. Is this on the heroku site or somewhere else? Sorry to be so dense!

@fontnerd it's not quite ready for you to look at yet. I've got the revised version in a separate branch in github -- which I guess I'm going to have to update with all the tweaks that have been made since I started working on it! (whoops) There were still some things that were obviously wrong with my conversion, so I was holding off.

Maybe I could ping you when it's merged back into master and ready for review? Or maybe I can do a little more cleanup and put it on my personal heroku dev site and you can look there first.

Contributor

scottkleinman commented Aug 7, 2013

I somehow missed Rebecca's post on this. I'll have a look and report back.
Sorry about the delay.

On 6 August 2013 18:15, Amy notifications@github.com wrote:

I'm a bit lost on how I can help here. Not quite sure what I should be
looking for. Is this on the heroku site or somewhere else? Sorry to be so
dense!


Reply to this email directly or view it on GitHubhttps://github.com/chnm/serendipomatic/issues/103#issuecomment-22223666
.

Scott Kleinman
Professor of English
Director, Center for the Digital Humanities
California State University, Northridge

rlskoeser pushed a commit that referenced this issue Aug 8, 2013

I decided my first attempt at this was too out of date now, with all the changes and tweaks people have been making. So I took a fresh stab at it based on the latest code, and went ahead and made the changes in master so we don't get out of sync this time. I think I got everything looking the same (and even fixed the offset footer on the results page), but would like our font & style experts to please review and confirm.

NOTE for anyone who needs to edit templates/html/css, things have moved slightly, as follows (comments also at the top of the templates):

  • site_base.html should have elements for all common elements across the site and is the basis for all other pages
  • page_base.html extends site_base.html and is the basis for all secondary pages (results page, about, connect, etc)
  • index_base.html extends site_base.html and is the basis for the site index page (doesn't really need to be separate, but I left it there since I think we're kind of used to that)

You should be aware that in any template that extends another template (which should now be everything but site_base.html), if you have any content outside of a {% block %} template tag, Django will ignore it. (I found some javascript for sorting on the results page that was like this-- went ahead and moved it in, although I don't think we're using it yet.) If we need to change something or add something outside of the current blocks, then we should define a new block in the top-level template. (If you don't know how to do this, ask someone and we should be able to set it up easily.)

I had a few detail questions from the template changes (I guess probably for @fontnerd and @scottkleinman?):

  • the apple touch icons were only included on the site index page; I assume that is intentional but wanted to check
  • I'm unfamiliar with this notation, but it's clearly doing something important for the styles; could someone give me a brief explanation or link me to some documentation that will help me understand?
<div class="sixteen columns" "top">
  • Another odd notation I noticed-- I didn't think multiple class attributes were allowed/valid, although (some? most?) browsers may just handle messy code and adapt. Is this intention or should it be cleaned up?
<div class="three columns" class="intro">

I think that's it. Sorry for the lengthy notes, but hopefully this will set us up for better consistency as we tweak the site design and content moving forward.

Contributor

scottkleinman commented Aug 8, 2013

Thanks for doing this, Rebecca. I'll pull the latest code tomorrow and have
a look.

As for your questions, I think some of the code (including the Apple touch
icons) was copied over from skeleton or, in the case of the two class
attributes, an oversight (should be class="three columns intro"). We should
try changing these things and see if anything breaks. I doubt it.

On 7 August 2013 20:22, Rebecca Sutton Koeser notifications@github.comwrote:

I decided my first attempt at this was too out of date now, with all the
changes and tweaks people have been making. So I took a fresh stab at it
based on the latest code, and went ahead and made the changes in master so
we don't get out of sync this time. I think I got everything looking the
same (and even fixed the offset footer on the results page), but would like
our font & style experts to please review and confirm.

NOTE for anyone who needs to edit templates/html/css, things have moved
slightly, as follows (comments also at the top of the templates):

  • site_base.html should have elements for all common elements across
    the site and is the basis for all other pages
  • page_base.html extends site_base.html and is the basis for all
    secondary pages (results page, about, connect, etc)
  • index_base.html extends site_base.html and is the basis for the site
    index page (doesn't really need to be separate, but I left it there since I
    think we're kind of used to that)

You should be aware that in any template that extends another template
(which should now be everything but site_base.html), if you have any
content outside of a {% block %} template tag, Django will ignore it. (I
found some javascript for sorting on the results page that was like this--
went ahead and moved it in, although I don't think we're using it yet.) If
we need to change something or add something outside of the current blocks,
then we should define a new block in the top-level template. (If you don't
know how to do this, ask someone and we should be able to set it up easily.)

I had a few detail questions from the template changes (I guess probably
for @fontnerd https://github.com/fontnerd and @scottkleinmanhttps://github.com/scottkleinman
?):

  • the apple touch icons were only included on the site index page; I
    assume that is intentional but wanted to check
  • I'm unfamiliar with this notation, but it's clearly doing something
    important for the styles; could someone give me a brief explanation or link
    me to some documentation that will help me understand? <div
    class="sixteen columns" "top">
  • Another odd notation I noticed-- I didn't think multiple class
    attributes were allowed/valid, although (some? most?) browsers may just
    handle messy code and adapt. Is this intention or should it be cleaned up?

I think that's it. Sorry for the lengthy notes, but hopefully this will
set us up for better consistency as we tweak the site design and content
moving forward.


Reply to this email directly or view it on GitHubhttps://github.com/chnm/serendipomatic/issues/103#issuecomment-22299813
.

Scott Kleinman
Professor of English
Director, Center for the Digital Humanities
California State University, Northridge

Contributor

mialondon commented Aug 8, 2013

I've pulled Rebecca's comments on the refactoring out as they're a good start for documenting the site for other developers - edit away at https://github.com/chnm/serendipomatic/wiki

@ghost ghost assigned fontnerd Aug 13, 2013

scottkleinman added a commit that referenced this issue Aug 14, 2013

JQuery loads only in site_base. Related to issue #103
Loads from CDN with local fallback.
Contributor

scottkleinman commented Aug 14, 2013

I have updated the files so that JQuery is loaded only from site_base. I would like to load JQuery and JQuery UI from CDN with a fallback to local resources. However, I am getting a "binary files" differ error when I try to push a local copy of JQuery UI. Does anyone know how to get around this?

@scottkleinman - not sure, does it tell you which file in particular is the problem? Make sure you've pulled the latest github repo to your local checkout and then check the diffs to make sure you're updating the correct files.

There are a few threads on stack overflow that might prove helpful; this one suggests you may have to merge the conflict manually: http://stackoverflow.com/questions/278081/resolving-a-git-conflict-with-binary-files

Contributor

scottkleinman commented Aug 17, 2013

I can't figure out which file is the problem, but it appears to be multiple
image files that trigger it. I'm going to try to push the files in small
batches to see if that works. Since we're not currently calling the JQuery
UI theme files locally, nothing should be affected.

On 16 August 2013 10:53, Rebecca Sutton Koeser notifications@github.comwrote:

@scottkleinman https://github.com/scottkleinman - not sure, does it
tell you which file in particular is the problem? Make sure you've pulled
the latest github repo to your local checkout and then check the diffs to
make sure you're updating the correct files.

There are a few threads on stack overflow that might prove helpful; this
one suggests you may have to merge the conflict manually:
http://stackoverflow.com/questions/278081/resolving-a-git-conflict-with-binary-files


Reply to this email directly or view it on GitHubhttps://github.com/chnm/serendipomatic/issues/103#issuecomment-22782476
.

Scott Kleinman
Professor of English
Director, Center for the Digital Humanities
California State University, Northridge

Contributor

scottkleinman commented Aug 17, 2013

The problem appears to have been something in the JQuery UI development bundle, which is not necessary if we are not changing anything. I was able to push the functional parts of JQuery UI so that the css can now be called locally. I think we can close this issue.

Contributor

mialondon commented Aug 18, 2013

Are there any testing instructions or documentation that needs to be finished to tidy up these changes?

Contributor

scottkleinman commented Aug 18, 2013

I am unaware of any particular testing instructions related to this issue,
but I have added some notes about JQuery usage to the documentation.

On 18 August 2013 06:17, Mia notifications@github.com wrote:

Are there any testing instructions or documentation that needs to be
finished to tidy up these changes?


Reply to this email directly or view it on GitHubhttps://github.com/chnm/serendipomatic/issues/103#issuecomment-22830301
.

Scott Kleinman
Professor of English
Director, Center for the Digital Humanities
California State University, Northridge

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