Moving jquery ui local and updating file structure (datepicker) #1733

clash99 · 2017-08-21T15:22:49Z

Proposed changes in this pull request

Partial of Retrofit handling of external files #1646
Moved jquery-ui files js, css and images locally
Combined css files into one
Adjusted paths
Modified js file to not use localization files and use transifex instead
This jquery ui is only being used for the datepicker function. In the future we may want to rethink this or see if there is other ways to leverage this library in the platform.

When should this PR be merged

When convenient

Risks

Low

Follow-up actions

One merged into master, need to update transifex translation files and verify js is working in datepicker as expected

Checklist (for reviewing)

General

Is this PR explained thoroughly? All code changes must be accounted for in the PR description.
- Review 1
- Review 2
Is the PR labeled correctly? It should have the migration label if a new migration is added.
- Review 1
- Review 2
Is the risk level assessment sufficient? The risks section should contain all risks that might be introduced with the PR and which actions we need to take to mitigate these risks. Possible risks are database migrations, new libraries that need to be installed or changes to deployment scripts.
- Review 1
- Review 2

Functionality

Are all requirements met? Compare implemented functionality with the requirements specification.
- Review 1
- Review 2
Does the UI work as expected? There should be no Javascript errors in the console; all resources should load. There should be no unexpected errors. Deliberately try to break the feature to find out if there are corner cases that are not handled.
- Review 1
- Review 2

Code

Do you fully understand the introduced changes to the code? If not ask for clarification, it might uncover ways to solve a problem in a more elegant and efficient way.
- Review 1
- Review 2
Does the PR introduce any inefficient database requests? Use the debug server to check for duplicate requests.
- Review 1
- Review 2
Are all necessary strings marked for translation? All strings that are exposed to users via the UI must be marked for translation.
- Review 1
- Review 2
Is the code documented sufficiently? Large and complex classes, functions or methods must be annotated with comments following our code-style guidelines.
- Review 1
- Review 2
Has the scalability of this change been evaluated?
- Review 1
- Review 2
Is there a maintenance plan in place?
- Review 1
- Review 2

Tests

Are there sufficient test cases? Ensure that all components are tested individually; models, forms, and serializers should be tested in isolation even if a test for a view covers these components.
- Review 1
- Review 2
If this is a bug fix, are tests for the issue in place? There must be a test case for the bug to ensure the issue won’t regress. Make sure that the tests break without the new code to fix the issue.
- Review 1
- Review 2
If this is a new feature or a significant change to an existing feature? has the manual testing spreadsheet been updated with instructions for manual testing?
- Review 1
- Review 2

Security

Confirm this PR doesn't commit any keys, passwords, tokens, usernames, or other secrets.
- Review 1
- Review 2
Are all UI and API inputs run through forms or serializers?
- Review 1
- Review 2
Are all external inputs validated and sanitized appropriately?
- Review 1
- Review 2
Does all branching logic have a default case?
- Review 1
- Review 2
Does this solution handle outliers and edge cases gracefully?
- Review 1
- Review 2
Are all external communications secured and restricted to SSL?
- Review 1
- Review 2

Documentation

Are changes to the UI documented in the platform docs? If this PR introduces new platform site functionality or changes existing ones, the changes must be documented in the Cadasta Platform Documentation.
- Review 1
- Review 2
Are changes to the API documented in the API docs? If this PR introduces new API functionality or changes existing ones, the changes must be documented in the API docs.
- Review 1
- Review 2
Are reusable components documented? If this PR introduces components that are relevant to other developers (for instance a mixin for a view or a generic form) they should be documented in the Wiki.
- Review 1
- Review 2

amplifi

🎉 Performance improvements!

oliverroick

Few questions:

Is there a minified version that we can use for jquery-ui.js? This PR removes jquery.min.js, which was 35.8KB (11.7KB zipped) and replaces it with an unminified version of 629KB (130KB zipped).
Are jquery-ui.structure.min.css and jquery-ui.theme.min.css folded into jquery-ui.min.css now. Or do we not need them any longer?
The datepicker doesn't seem to be translated any longer. So when I select Spanish as my default language, I still get the English date picker. (Though it seems like this doesn't work properly on staging either).

clash99 · 2017-08-25T13:30:07Z

@oliverroick -

There is a js minified version but in order to avoid all the jquery-ui-specific i18n files (which seem to keep breaking), I've updated the js to use our transifex translation. As a follow-up to this task, I'm going to look at optimizing all our static files at once.
Yes, I've combined the css files into one. We no longer need three for the jquery-ui.
The translations will need to be "sucked" into transifex, translated, and then pulled back out for it to work properly. I will update all the transifex files once this has been merged into the master branch. Yes the datepicker translation it is currently broken on staging.

oliverroick

@clash99 Thanks for clarifying. Do we have a follow-up ticket filed to implement processes to minify JS and CSS files, or should I open one?

Jquery UI images Adjust i18n path Sidestep date picker localization files and use transifex Removed unnecessary type from script tags Combined jquery-ui css files

seav · 2017-09-06T15:34:31Z

cadasta/templates/spatial/relationship_add.html

@@ -45,6 +42,33 @@
        changeYear: true,
      });
    });
+=======


This looks like a rebase conflict that wasn't resolved properly.

seav · 2017-09-06T15:35:17Z

cadasta/templates/spatial/location_add.html

-<script src="{% static 'js/jquery-ui.min.js' %}"></script>
-<script src="https://cdn.rawgit.com/jquery/jquery-ui/1.12.1/ui/i18n/datepicker-{{ LANGUAGE_CODE }}{% if LANGUAGE_CODE == 'en' %}-GB{% endif %}.js"></script>
+<script src="{% static 'jquery-ui/1.12.1/js/jquery-ui.js' %}"></script>
+<script src="{% static 'js/form_submit.js' %}"></script>


This line now appears twice (see line 31).

amplifi added PR: needs review PR: needs testing labels Aug 22, 2017

amplifi approved these changes Aug 22, 2017

View reviewed changes

amplifi requested a review from oliverroick August 25, 2017 10:48

oliverroick reviewed Aug 25, 2017

View reviewed changes

oliverroick approved these changes Aug 28, 2017

View reviewed changes

oliverroick added PR: approved and removed PR: needs review labels Aug 28, 2017

Moving jquery ui local and updating file structure

d33c99b

Jquery UI images Adjust i18n path Sidestep date picker localization files and use transifex Removed unnecessary type from script tags Combined jquery-ui css files

amplifi force-pushed the feature/local-jquery-ui branch from 4ef1c6b to d33c99b Compare September 6, 2017 11:26

amplifi merged commit 3feaec9 into master Sep 6, 2017

amplifi deleted the feature/local-jquery-ui branch September 6, 2017 11:57

seav reviewed Sep 6, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Moving jquery ui local and updating file structure (datepicker) #1733

Moving jquery ui local and updating file structure (datepicker) #1733

clash99 commented Aug 21, 2017

amplifi left a comment

oliverroick left a comment

clash99 commented Aug 25, 2017

oliverroick left a comment

seav Sep 6, 2017

seav Sep 6, 2017

Moving jquery ui local and updating file structure (datepicker) #1733

Moving jquery ui local and updating file structure (datepicker) #1733

Conversation

clash99 commented Aug 21, 2017

Proposed changes in this pull request

When should this PR be merged

Risks

Follow-up actions

Checklist (for reviewing)

General

Functionality

Code

Tests

Security

Documentation

amplifi left a comment

Choose a reason for hiding this comment

oliverroick left a comment

Choose a reason for hiding this comment

clash99 commented Aug 25, 2017

oliverroick left a comment

Choose a reason for hiding this comment

seav Sep 6, 2017

Choose a reason for hiding this comment

seav Sep 6, 2017

Choose a reason for hiding this comment