New jsviewer css by jfoster17 · Pull Request #2982 · astropy/astropy

jfoster17 · 2014-09-25T21:29:24Z

In response to #2917, I have swapped in a lighter-weight CSS for the jsviewer table output. This also has some features like subtle row-highlighting (based on mouse-over) and column-highlighting (based on sort). Here's a short example (sorted on nobs and hovering over the 5th row).

The only problem is that I had to remove the in-line table styling jfoster17@c1ea7b7
in order to make this table style look nice, which means that if the CSS doesn't load from the remote server, the fallback styling is normally pretty bad (this is browser/configuration dependent). Perhaps someone else knows how to fix this?

eteq · 2014-09-26T15:07:35Z

cc @keflavich @astrofrog @kbarbary re: the question at the end

eteq · 2014-09-26T15:08:32Z

You might want to update this to [#2982] for this PR. Or [#2917, #2982]

jfoster17 · 2014-09-26T15:33:12Z

Actually removing the default style:

table,th,td,tr,tbody {border: 1px solid black; border-collapse: collapse;}

is causing the tests to fail (I ran the tests wrong prior to issuing this PR) because it's hard-coded in the write tests as well. I'm going to restore that for now so people can comment on the new style.

astrofrog · 2014-09-27T09:14:36Z

I like the new style, but I was wondering if it would be possible to use a sans-serif font? It might make it look even more light-weight. We could even use the web font 'Source Sans Pro' which is used on the astropy website.

jfoster17 · 2014-09-27T20:32:50Z

@astrofrog, I certainly approve of using a sans-serif font for small font-sizes on a screen. In order to keep things simple, I just found I template I liked that was already hosted on a CDN, but I'd be all for modifying it.

This raises a larger question, though, which was obliquely mentioned in the comments on PR #2875 but didn't get a full discussion.

There are, as I see it, 5 different options for styling an HTML table.

Include the CSS and JS in the same file as the HTML
- Good: Works everywhere, easily portable
- Bad: Bad practice. Makes HTML files hard to parse. Makes it hard for people to use any other style to look at their files.
Output CSS and JS files alongside the HTML (this is what include_js_files = True should do)
- Good: Always works (as long as files are moved along with the HTML)
- Bad: User is required to understand/honor the filepath structure between the HTML file and associated JS/CSS.
Fetch JS/CSS remotely from reliable server/CDN (current system)
- Good: No extra files to worry about
- Bad: File formatting is (temporarily) lost if user does not have a network connection. Limited to pre-defined templates.
Fetch JS/CSS remotely from astropy website. This is just like option 3 except...
- Good: Can tweak templates to our satisfaction
- Bad: Formatting is lost if astropy server is down
Store local copy of JS/CSS in the astropy source directory
- Good: Works seamlessly on computers with astropy installed
- Bad: Formatting lost on machine without astropy (or older/different version)

I don't see a perfect choice among these different systems. Option 1 is certainly the least ideal from a coding perspective. The other options are not mutually exclusive -- we could have the page try to get a remote CSS file and, if that files, try to get a local copy (either in the current directory or in the astropy installation directory).

jfoster17 · 2014-10-03T00:09:34Z

Okay, so it seems pretty simple to have a fallback scheme whereby we try to find the CSS/JS

In the current directory with the HTML file
Hosted on some website (CDN for JS, astropy? for our modified CSS)
In the astropy source tree

That should work in the largest number of cases. Additional commits to implement this are coming once I figure out where to host the modified CSS.

@astrofrog, in #2875 you said:

"If we find a style we like better, such as the one you pointed out, we can always host the CSS file ourselves if it doesn't already exist somewhere reliable."

Where would we do this? On the astropy website? Would we just add a new CSS file to astropy.github.com? I just need to host a slightly modified version of the CSS to implement a better font and a few other cosmetic things.

astrofrog · 2014-11-05T09:00:25Z

@jfoster17 - sorry I dropped the ball on this (this will need rebasing, presumably due to the changelog). Regarding your question, we could consider hosting the style on data.astropy.org but for now that infrastructure is still undergoing changes so we need to be patient on that front.

+1 to the fallback scheme you mention (same directory, then remote, then astropy source tree).

jfoster17 · 2014-11-05T22:21:36Z

Okay, I've rebased, so this should be good-to-go. I'll wait on astropy.data.org before making the CSS/JS stuff have a robust fallback scheme.

eteq · 2014-11-05T23:03:05Z

👍 from me on this.

One quick question: in this PR, is the behavior 1) and if missing try 3) from above? I guess I'm not actually clear which (if any) of those options is in current master...

jfoster17 · 2014-11-06T15:34:57Z

In this PR and in the current master, there is essentially no fallback behavior. The written HTML file looks at a specified URL for the JS/CSS (option 2). If it is not found, there is some simple in-line HTML styling so that the table is legible, but you lose all the interactive JS-goodness.

saimn · 2015-09-08T08:38:51Z

As these classes are options of the DataTables stylesheet and can be combined in different ways, it would be great to have the possibility to specify this in the show_in_browser method. http://www.datatables.net/manual/styling/classes

saimn · 2015-09-08T08:43:33Z

What's the status on this ? I'm interested in using the table's HTML output in a web interface, so having a nice and flexible style would be great, and I could also have a look a #4058 but to avoid conflicts it would be better to finalize this PR first. Anything I can to do to help ?

taldcroft · 2015-09-13T22:40:05Z

@saimn - I'm assigned to review this but have been swamped. Can I delegate testing this out and doing the final review to you? That would certainly help.

I do note that this PR needs rebasing at this time.

taldcroft · 2015-09-13T22:41:19Z

I'm not sure why this comment is being removed here, perhaps it is a funny that is related to the required rebase.

It seems it was removed in f2c1567 This should definitely be fixed.
It can be fixed by the following process:

Run $ git rebase -i master

In the text file that comes up, mark the commit f2c1567 as "edit" (that is, replace the word "pick" with "edit")

Save the file; exit. The rebase will proceed until f2c1567 and then pause.

Run $ git reset HEAD^--this will basically undo that commit. Typing $ git status will show the changes in that commit as though it were not saved yet.

You can edit the commit manually to restore the removed line, or run $ git checkout --patch to selectively undo edits (keep the added line, discard the change that removes the accidentally removed lines).

Save the new commit (with its original commit message) by running $ git commit -C ORIG_HEAD

Complete the rebase: $ git rebase --continue

Push the new changeset: $ git push --force origin new-jsviewer-css

🍻

saimn · 2015-09-19T23:03:41Z

I have tested and rebased the branch on master: https://github.com/saimn/astropy/commits/2982-rebased
To enhance this PR I did some cleanup, upgraded js files, added an option to set the table class, and added the DataTables css file which was missing when using the use_local_files option.
I will try to polish this tomorrow and send a new PR, if it seems a good way to proceed, and unless @jfoster17 wants to take on this ? Btw I kept all the original commits.

taldcroft · 2015-09-20T14:08:52Z

@saimn - it looks like you can take this over with your updated branch. Thanks!

saimn · 2015-09-26T13:53:32Z

@taldcroft This one can be closed :-)

eteq reviewed Sep 26, 2014
View reviewed changes

Comment thread CHANGES.rst Outdated

eteq Sep 26, 2014

Copy link
Copy Markdown

Member

Choose a reason for hiding this comment

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

You might want to update this to [#2982] for this PR. Or [#2917, #2982]

jfoster17 added 6 commits November 5, 2014 16:54

New lightweight CSS for jsviewer table output

3160790

Remove default in-line table style. Good if CSS loads, bad otherwise.

1c74265

Add new CSS description to CHANGES

553dd01

Restore in-line table style and other updates to fix tests

06cda68

CHANGES.rst now refers to this PR as well

f2c1567

Fixing PEP8 whitespace

dd79ec3

jfoster17 force-pushed the new-jsviewer-css branch from 35fa150 to dd79ec3 Compare November 5, 2014 22:20

astrofrog added the table label Mar 21, 2015

astrofrog assigned taldcroft Mar 21, 2015

saimn reviewed Sep 8, 2015
View reviewed changes

taldcroft reviewed Sep 13, 2015
View reviewed changes

saimn mentioned this pull request Sep 20, 2015

New jsviewer css [rebased] #4174

Merged

taldcroft closed this Sep 26, 2015

Conversation

jfoster17 commented Sep 25, 2014

Uh oh!

eteq commented Sep 26, 2014

Uh oh!

eteq Sep 26, 2014

Choose a reason for hiding this comment

Uh oh!

jfoster17 commented Sep 26, 2014

Uh oh!

astrofrog commented Sep 27, 2014

Uh oh!

jfoster17 commented Sep 27, 2014

Uh oh!

jfoster17 commented Oct 3, 2014

Uh oh!

astrofrog commented Nov 5, 2014

Uh oh!

jfoster17 commented Nov 5, 2014

Uh oh!

eteq commented Nov 5, 2014

Uh oh!

jfoster17 commented Nov 6, 2014

Uh oh!

saimn Sep 8, 2015

Choose a reason for hiding this comment

Uh oh!

saimn commented Sep 8, 2015

Uh oh!

taldcroft commented Sep 13, 2015

Uh oh!

taldcroft Sep 13, 2015

Choose a reason for hiding this comment

Uh oh!

embray Sep 15, 2015

Choose a reason for hiding this comment

Uh oh!

saimn commented Sep 19, 2015

Uh oh!

taldcroft commented Sep 20, 2015

Uh oh!

saimn commented Sep 26, 2015

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants