chnm / serendipomatic

http://serendipomatic.org/
26 stars 9 forks source link

consolidate/combine site_base and view_base templates #103

Closed rlskoeser closed 11 years ago

rlskoeser commented 11 years ago

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.

scottkleinman commented 11 years ago

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.

rlskoeser commented 11 years ago

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.

mialondon commented 11 years ago

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.

scottkleinman commented 11 years ago

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

rlskoeser commented 11 years ago

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

fontnerd commented 11 years ago

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!

rlskoeser commented 11 years ago

@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.

scottkleinman commented 11 years ago

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 commented 11 years ago

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):

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?):

<div class="sixteen columns" "top">
<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.

scottkleinman commented 11 years ago

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? <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.

— 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

mialondon commented 11 years ago

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

scottkleinman commented 11 years ago

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?

rlskoeser commented 11 years ago

@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

scottkleinman commented 11 years ago

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

scottkleinman commented 11 years ago

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.

mialondon commented 11 years ago

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

scottkleinman commented 11 years ago

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