Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#6865 closed enhancement (fixed)

[with patch, positive review] Use templates for CSS

Reported by: timdumol Owned by: boothby
Priority: major Milestone: sage-4.1.2
Component: notebook Keywords: notebook css stylesheets
Cc: Merged in: Sage 4.1.2.alpha4
Authors: Tim Dumol Reviewers: Mitesh Patel, Jason Grout
Report Upstream: Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description

CSS is currently served by css() in css.py, even though it is completely static. Using templates for it should make things easier to customize in the future.

Attachments (3)

trac_6865-templates-css.patch (68.1 KB) - added by timdumol 5 years ago.
Moved CSS in css.py to templates, and adjusted twist.py to use them.
trac_6865-templates-css.2.patch (67.5 KB) - added by timdumol 5 years ago.
Moved CSS in css.py to templates, and adjusted twist.py to use them. Rev 2. Apply this patch only.
trac_6865-templates-css.3.patch (67.8 KB) - added by mpatel 5 years ago.
Moved CSS in css.py to templates, and adjusted twist.py to use them. Rev 3. Apply this patch only.

Download all attachments as: .zip

Change History (17)

Changed 5 years ago by timdumol

Moved CSS in css.py to templates, and adjusted twist.py to use them.

comment:1 Changed 5 years ago by timdumol

  • Authors set to Tim Dumol
  • Milestone set to sage-4.1.2
  • Summary changed from Use templates for CSS to [with patch, needs review] Use templates for CSS

comment:2 Changed 5 years ago by jason

This probably needs to be rebased after #6939 to include the CSS fix there (or the rebasing should go the other way, if this gets reviewed before that gets merged...)

comment:3 Changed 5 years ago by jason

#6940 is a dup of this ticket (i.e., close #6940).

comment:4 Changed 5 years ago by mpatel

Does the patch depend on another ticket's patch? Applying trac_6865-templates-css.patch to 4.1.2.alpha1, I get

applying trac_6865-templates-css.patch
patching file sage/server/notebook/css.py
Hunk #1 FAILED at 0
1 out of 3 hunks FAILED -- saving rejects to file sage/server/notebook/css.py.rej
patching file sage/server/notebook/twist.py
Hunk #1 FAILED at 133
Hunk #2 succeeded at 1787 with fuzz 1 (offset 37 lines).
Hunk #3 FAILED at 2315
2 out of 3 hunks FAILED -- saving rejects to file sage/server/notebook/twist.py.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh trac_6865-templates-css.patch

The important one is the last, which doesn't apply because the local twist.py contains

        elif self.problem == 'suspended':
            return HTMLResponse(stream = message("Your account is currently suspended."))

But I could have altered my configuration.

comment:5 Changed 5 years ago by mpatel

I think we need to add

include sage/server/notebook/css/*

to MANIFEST.in, or we'll definitely hear from the release manager...

comment:6 follow-up: Changed 5 years ago by jason

What is MANIFEST.in and when do we need to add things to it?

comment:7 Changed 5 years ago by jason

  • Summary changed from [with patch, needs review] Use templates for CSS to [with patch, needs work] Use templates for CSS

(setting to "needs work", based on mpatel's comment about MANIFEST.in and his comment about applying the patch)

comment:8 in reply to: ↑ 6 Changed 5 years ago by mpatel

Replying to jason:

What is MANIFEST.in and when do we need to add things to it?

I believe the sage -bdist and sage -sdist invoke distutils to build sage-*.spkg. Distutils uses SAGE_ROOT/devel/sage/MANIFEST.in to determine which files to include in the distribution. If we add files to the local Mercurial repository, we should check that some line(s) in MANIFEST.in includes the newly tracked files or add a new line. Otherwise, the next (pre-)release of Sage will not include them. As mvngu puts it, this results in repository corruption.

Previous sightings: #4460, #5653, #5799, #6568, etc.

comment:9 Changed 5 years ago by timdumol

  • Summary changed from [with patch, needs work] Use templates for CSS to [with patch, needs review] Use templates for CSS

No, this doesn't depend on other patches. It is based on 4.1.2.alpha1.

I've made the requisite changes to MANIFEST.in.

Changed 5 years ago by timdumol

Moved CSS in css.py to templates, and adjusted twist.py to use them. Rev 2. Apply this patch only.

Changed 5 years ago by mpatel

Moved CSS in css.py to templates, and adjusted twist.py to use them. Rev 3. Apply this patch only.

comment:10 Changed 5 years ago by mpatel

  • Summary changed from [with patch, needs review] Use templates for CSS to [with patch, partial positive review] Use templates for CSS

Changes in patch v3:

  • css.py: if os.path.exists(user_css): --> if os.path.exists(user_css_path):
  • css.py: + --> os.path.join()
  • css.py: html -> HTML in docstring
  • main.css: {% -> {%-

Sorry about the trivial stuff. I just noticed the whitespace control part of the Jinja2 docs.

This is a positive review from me, but someone should review my changes.

comment:11 Changed 5 years ago by jason

  • Summary changed from [with patch, partial positive review] Use templates for CSS to [with patch, positive review] Use templates for CSS

Your changes look good to me.

comment:12 Changed 5 years ago by jason

  • Priority changed from minor to major

comment:13 Changed 5 years ago by mvngu

  • Merged in set to Sage 4.1.2.alpha3
  • Resolution set to fixed
  • Reviewers set to Mitesh Patel, Jason Grout
  • Status changed from new to closed

Merged trac_6865-templates-css.3.patch.

comment:14 Changed 5 years ago by mvngu

  • Merged in changed from Sage 4.1.2.alpha3 to Sage 4.1.2.alpha4

There is no 4.1.2.alpha3. Sage 4.1.2.alpha3 was William Stein's release for working on the making the notebook a standalone package.

Note: See TracTickets for help on using tickets.