Skip to content

Add source files for jQuery and dataTables. - #4334

Merged
eteq merged 3 commits into
astropy:masterfrom
saimn:js-sources
Nov 23, 2015
Merged

Add source files for jQuery and dataTables.#4334
eteq merged 3 commits into
astropy:masterfrom
saimn:js-sources

Conversation

@saimn

@saimn saimn commented Nov 20, 2015

Copy link
Copy Markdown
Contributor

@saimn
Add source files for jQuery and dataTables.
8aeb7c5 
+ Ref astropy#4330: source files are needed for Debian packaging.
+ Use the non-minified version for the dataTables css as the gain
is negligible.
@saimn

saimn commented Nov 20, 2015

Copy link
Copy Markdown
Contributor Author

For the record: DataTables 1.10.10 is available but I have added the source for 1.10.9 which correspond to the minified version already present in astropy.
Version 1.10.10 bring some changes for the AMD module definition which is used by the show in notebook feature, so I think it is better to keep this version for later.
See http://datatables.net/feeds/releases.xml for the changelog (no really practical by it is the only link I found).

@embray embray added Affects-dev PRs and issues that do not impact an existing Astropy release build table labels Nov 20, 2015
@embray embray added this to the v1.1.0 milestone Nov 20, 2015
@embray

embray commented Nov 20, 2015

Copy link
Copy Markdown
Member

Thanks for the quick turn-around on this @saimn !

@eteq

eteq commented Nov 22, 2015

Copy link
Copy Markdown
Member

Great, thanks @saimn!

One suggestion: maybe add a README to the extern/js directory that explains that the minified files are the ones that are actually used, but the non-minified versions are there so that the full source code is present in the distribution? That way hopefully we can avoid this happening again.

Also, @olebole (and maybe @sergiopasra if you have a moment), can you confirm that this now confirms to the requirements for packaging?

@olebole

olebole commented Nov 22, 2015

Copy link
Copy Markdown
Member

Yes, thanks! This is what I needed for packaging.

@saimn

saimn commented Nov 23, 2015

Copy link
Copy Markdown
Contributor Author

One suggestion: maybe add a README to the extern/js directory that explains that the minified files are the ones that are actually used, but the non-minified versions are there so that the full source code is present in the distribution? That way hopefully we can avoid this happening again.

Yes, good idea, I'm going to add this.

@astrofrog

astrofrog commented Nov 23, 2015

Copy link
Copy Markdown
Member

@saimn - thanks! Would it be possible to do this today so that we can include it in the release candidate?

@saimn

saimn commented Nov 23, 2015

Copy link
Copy Markdown
Contributor Author

@astrofrog oh yes, of course, I take a look.

@saimn

saimn commented Nov 23, 2015

Copy link
Copy Markdown
Contributor Author

Actually there is already a README in extern/ , with a note about JQuery / DataTables for packagers, so it make sense to a note for developers about (non-)minified files, right ?

@saimn

saimn commented Nov 23, 2015

Copy link
Copy Markdown
Contributor Author

Done, is the formulation ok ?

astrofrog
astrofrog reviewed Nov 23, 2015
View reviewed changes
Comment thread astropy/table/jsviewer.py

@astrofrog astrofrog Nov 23, 2015

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.

I thought we were using the minified ones? Why change the default? (and same comment below)

@eteq eteq Nov 23, 2015

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.

For the CSS, I think @saimn noted above that the minified version is not much smaller than the regular one so there seems little point in having the minified version at all. The source, though, should be the mins (which are quite a bit smaller/more optimized)

@astrofrog astrofrog Nov 23, 2015

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.

Ah ignore me, didn't realize this was the CSS not the JS

@astrofrog

astrofrog commented Nov 23, 2015

Copy link
Copy Markdown
Member

👍 from me

@eteq
eteq merged commit 6316d7c into astropy:master Nov 23, 2015
eteq added a commit that referenced this pull request Nov 23, 2015
@eteq
Merge pull request #4334 from saimn/js-sources
1663cca 
Add back in full source files for jQuery/dataTables

Includes manual tweaks of README
eteq added a commit to eteq/astropy that referenced this pull request Nov 23, 2015
@eteq
Merge pull request astropy#4334 from saimn/js-sources
1f268d5 
Add back in full source files for jQuery/dataTables

Includes manual tweaks of README
@eteq

eteq commented Nov 23, 2015

Copy link
Copy Markdown
Member

Merged! And backported to v1.1.x in 1f268d5

@eteq eteq mentioned this pull request Nov 23, 2015
Closed
@saimn
saimn deleted the js-sources branch November 24, 2015 21:26
@bsipocz bsipocz mentioned this pull request Nov 29, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

Affects-dev PRs and issues that do not impact an existing Astropy release build table

Projects

None yet

Milestone

v1.1.0

Development

Successfully merging this pull request may close these issues.

5 participants

@saimn @embray @eteq @olebole @astrofrog