Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update jquery and django #906

Merged
merged 5 commits into from
Aug 27, 2019
Merged

Update jquery and django #906

merged 5 commits into from
Aug 27, 2019

Conversation

Jkrzy
Copy link
Contributor

@Jkrzy Jkrzy commented Jun 26, 2019

Description

Per #905, Updating our vendored version of jquery.min.js to the latest version. Also updating documentation to reflect our use of NPM alongside vendored dependencies.

@Jkrzy Jkrzy requested review from amymok and tbaxter-18f June 26, 2019 17:56
@Jkrzy Jkrzy changed the title Update jquery Update jquery and django Jul 2, 2019
@codecov-io
Copy link

codecov-io commented Jul 2, 2019

Codecov Report

Merging #906 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #906   +/-   ##
=======================================
  Coverage   91.66%   91.66%           
=======================================
  Files          39       39           
  Lines        1668     1668           
=======================================
  Hits         1529     1529           
  Misses        139      139

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5801541...dcebc5b. Read the comment docs.

amymok
amymok previously approved these changes Jul 2, 2019
With this commit we move all of our vendored js/css
dependencies into a single folder. We're also renaming
w/ versions and making the necessary template changes
to update paths.

This will give us greater visibilty into the current versions
being served by tock.

As part of this cleanup, remove datatables.js entirely
as it is not currently in use.
var $table = $('#dataTable').dataTable({
scrollX: true,
paging: false,
lengthChange: false
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This line contained a syntax error which has prevented datatables from running. It doesn't look like this has worked for quite some time as resolving the syntax breaks styling on the page. Herein we remove in favor of fixing.

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe this is an opportunity to remove the datatable dependency altogether?

I'd love it if we could get to a point where we didn't have a jquery dependency, either. This might be one step in that direction?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are 😄, that code snippet and the source files associated with datatables are removed w/ this PR.

@Jkrzy
Copy link
Contributor Author

Jkrzy commented Aug 8, 2019

Hey @amymok, just pushed another update here for latest dependencies as well as a general clean-up of our vendor'd things being served out of static/. Please take a look when you have a moment. Thanks!

An additional note on the removal of datatables.js here: dcebc5b#r312072292

@Jkrzy Jkrzy requested a review from amymok August 8, 2019 14:40
@Jkrzy Jkrzy dismissed amymok’s stale review August 8, 2019 14:40

Need an updated review, thanks!

Copy link
Member

@amymok amymok left a comment

Choose a reason for hiding this comment

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

This looks good to me!

@Jkrzy Jkrzy merged commit 58b51c2 into master Aug 27, 2019
@Jkrzy Jkrzy deleted the js-updates branch August 27, 2019 14:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants