Add simple Django based domain selection view #700
Conversation
| </form> | ||
|
|
||
| <script> | ||
| startup_scripts.push(loadFabricNames); |
There was a problem hiding this comment.
I love seeing this get used. Good one.
| } | ||
|
|
||
| function loadFabricDomain(event) { | ||
| var name = $("#fabric-selector").val(), |
There was a problem hiding this comment.
Use let instead of var and try to keep one declaration per statement.
let name = ...;
let type = ...;
let catLists = ...;Looks heavy, but the cognitive load is lower and it makes it easier to edit later on.
| for (l = 0; l < catLists.length; l++) { | ||
| select = catLists[l]; | ||
| for (i = select.options.length - 1; i >= 0; i--) { | ||
| select.remove(i); | ||
| } | ||
| } |
There was a problem hiding this comment.
Try something like:
for (let catchmentSelect of catLists) {
for (let optionIndex = catchmentSelect.options.length - 1; optionIndex >= 0; optionIndex--) {
catchmentSelect.remove(optionIndex);
}
}I may just be easier to do $("#domainChoices option, #domainSelections option").remove(), though.
| @@ -0,0 +1,206 @@ | |||
|
|
|||
There was a problem hiding this comment.
Something I'm seeing that you should try is to use let instead of var, declare one variable per statement, avoid acronyms (such as i and l), use for (let x of y) statements instead of classical for loops, and maybe take advantage of jQuery more when it comes to iterating over collections of stuff like options.
Don't sleep on .forEach on lists either. Super useful.
There was a problem hiding this comment.
I've gone through and changed the multi-variable vars to single variable lets, along with a few small tweaks to variable names.
There was a problem hiding this comment.
Feel comfortable adding some jsdoc? I understand if you're running to get all this in there so you can move on, but if you feel it's reasonable, jsdoc entries help with stuff like type hinting and will make it easier for others to work on this in the future.
| }).join(' '); | ||
| } | ||
|
|
||
| function loadFabricNames() { |
There was a problem hiding this comment.
This would be a great place to get used to the fetch API. $.ajax does the job, but if you want to experiment a little, this is a great way to get used to async javascript in a small controlled manner.
There was a problem hiding this comment.
This derived directly from a similar function in map.js. I don't really want to perfect the implementation at this stage, especially while trying to apply something considerably different (that I'm already not especially familiar with). I'm going to leave this alone unless you feel like there is a really compelling reason to try to do this now.
There was a problem hiding this comment.
Hey, I wrote that before I knew how to use fetch!
I don't really want to perfect the implementation at this stage, especially while trying to apply something considerably different (that I'm already not especially familiar with)
Great decision. Can you add a @todo near it or a ticket for converting traditional ajax calls to the fetch api? Chances of addressing the TODO are probably low, but at least it'll be in there as low hanging fruit for anyone that might be interested.
| success: function(result,status,xhr) { | ||
| if (result != null) { | ||
| result['fabric_names'].forEach(function(name) { | ||
| $("#fabric-selector").append("<option value='" + name + "'>" + titleCase(name) + "</option>"); |
There was a problem hiding this comment.
This can be refactored to look a little more like:
$("#fabric-selector").append(`<option value="${name}">${titleCase(name)}</option>`);| @@ -173,6 +173,8 @@ def get_full_localtimezone(): | |||
| BASE_DIRECTORY = Path(__file__).resolve().parent.parent | |||
| """The path to the base directory of the service""" | |||
|
|
|||
| STATIC_ROOT = BASE_DIRECTORY / "static" | |||
|
|
|||
| STATIC_RESOURCE_DIRECTORY = BASE_DIRECTORY / "static" / "resources" | |||
There was a problem hiding this comment.
May as well construct this with the new definition for STATIC_ROOT.
| for editor in configuration.get_editors(): | ||
| framework_selector.add_choice(editor['name'], editor['description'], editor['friendly_name']) | ||
|
|
||
| pprint(framework_selector.__dict__) |
There was a problem hiding this comment.
Did you mean for this debug statement to stay in here?
| :return: A rendered page | ||
| """ | ||
| # If a list of error messages wasn't passed, create one | ||
| if 'errors' not in kwargs: |
There was a problem hiding this comment.
A list of errors and stuff wouldn't get passed in here - it would be path variables. If the path to the domain view was something along the lines of /domain/(?P<one>...)/(?P<two>...)/(?P<three>...), I believe the key value pairs for one, two, and three would end up in kwargs. Things like list of errors, warnings, and info would come in through request.GET.
| type = request.GET.get('fabric_type', 'catchment') | ||
| if not type: | ||
| type="catchment" | ||
| type = "catchment" |
There was a problem hiding this comment.
It might be wise to abstract out this logic into another function that would be somewhat "easy" to swap in and out. Won't the fabrics come via the data store in the future?
| if isinstance(id_only, str): | ||
| id_only = id_only.strip().lower() == "true" | ||
| else: | ||
| id_only = bool(id_only) |
There was a problem hiding this comment.
bool("false") == True
There was a problem hiding this comment.
There is a helper function in dmod.core.common that can help identify truthy values from a string. I believe it's called is_true.
| @@ -173,6 +173,8 @@ def get_full_localtimezone(): | |||
| BASE_DIRECTORY = Path(__file__).resolve().parent.parent | |||
| """The path to the base directory of the service""" | |||
|
|
|||
| STATIC_ROOT = BASE_DIRECTORY / "static" | |||
There was a problem hiding this comment.
Has this been tested inside and outside of docker? Setting Django's STATIC variables can end up having odd or ill or differing effects when placed behind nginx due to routing issues.
| @@ -0,0 +1,118 @@ | |||
| {% extends 'base.html' %} | |||
There was a problem hiding this comment.
May want to inherit from master.html instead.
| @@ -34,6 +35,8 @@ | |||
| # packet sniffer and use the cookie to hijack the user’s session. | |||
| SESSION_COOKIE_SECURE = not DEBUG | |||
|
|
|||
| CSRF_TRUSTED_ORIGINS = os.environ.get('TRUSTED_ORIGINS', '').split(',') | |||
There was a problem hiding this comment.
Has this been tested behind docker with and without a cert and local vs remote? Playing with origins may cause issues when working between machines on different hosts or when working locally outside of a docker instance.
|
I suggest using a grid display with the height filling the entire viewable area within the containing box, with the hydrofabric fieldset in row 1 through column 3, the catchment select in row 2 column 1, the buttons for selecting what to move around in row 2 column 2, and the selected catchments in row 2 column 3. Either that or a flex box will make it easier to take advantage of your viewable area. |
|
Is there a name for the front end pattern of having one box of values that may move over to the other that's in this PR? It's ultra common, so I wonder if there's a simple implementation that we can import for use. |
Marking several utility functions (used by some broken places in GUI code) as deprecated, largely because they don't actually work.
Adding required STATIC_ROOT to be main static directory, and removing the addition of this directory in things scanned by the static file finders (i.e., since root, have other found things put here); this was needed to fix issues with static files from main model exec GUI app, which were not being found with previous config.
Adding daphne to dependencies.txt to install this requirment in GUI image build.
Moving base maas config html template to use css/master.css instead of common/css/base.css for consistent presentation.
Fixing some GUI classes that were broken and not update after changes to dmod.communication package, and then re-added URL to Django config that were taken out due to the previous classes breaking.
Hard-coding reasonable defaults for superuser setup values needed to get previously existing migration to work.
Making sure to put it after lines that export the SQL_PASSWORD as needed.
Cleaning up existing view implementations, making default example match example data in repo, adding support to only query for feature ids, and adding support for geopackage-type hydrofabrics.
Adding most of what is required for basic working domain selection view that can select hydrofabrics and the desired catchments to execute from the hydrofabric, though not yet with the form fields to proceed to the next job config step.
robertbartel
force-pushed
the
f/revise_django_gui/domain_view
branch
from
f3b57c0
to
860a3c3
Compare
Switching from use of var and combined declarations; also refactoring a few variable names for clarity, per suggestions.
robertbartel
force-pushed
the
f/revise_django_gui/domain_view
branch
from
cd6e8bf
to
d62ea95
Compare
Addition of a simple form-based domain selection view allowing for selecting from the available hydrofabrics and the available catchments in the selected hydrofabric before transitioning to the formulation configuration view.
Depends upon #695 so should remain a draft until that is resolved.