Dst core/develop #21
Dst core/develop #21
Conversation
doesn't work great.. but proof of concept is solid. TODO: update map pin when geolocate is selected. Find out why most of the time (90%) first attempt to geolocate fails.
and its not going well
…0 years old. If system is old, summary report now includes a sentence to be aware of fact that climate change may have already changed a lot (with implication that system may be already maladapted).
Jeremyfyke/sandbox
Jeremyfyke/sandbox
changes: -added new (placeholder, not license-tested) images -added script to auto-crop input images to defined aspect ratio -added new logic to allow for images in accordion box summary report.
.gitignore
Outdated
| .DS_Store | ||
| *.py |
There was a problem hiding this comment.
Not a good idea to ignore the generated .py as this file is meant to ease the PR reviews.
.pre-commit-config.yaml
Outdated
| - id: jupytext | ||
| name: "Generate a .py file from the notebook" | ||
| entry: jupytext --to py decision-support-tool.ipynb | ||
| pass_filenames: false |
There was a problem hiding this comment.
Please keep this file. If you absolutely need to deactivate, simply uninstall pre-commit from your local machine. This validation might be sometimes annoying for small changes/wip, but before deploying it's really important to make sure nothing broke and we don't have regression. I would say that you can bypass this system for feat-branch -> develop, but for develop -> master I prefer to keep this mecanism.
There was a problem hiding this comment.
Thanks for these. Will revert these changes, and work on workflow on my end as per your comments.
There was a problem hiding this comment.
@JeremyFyke Two little things to change but overall good PR. Once you'll have made the modification, I will deploy the new version.
-added .pre0commit-config.yaml back -removed '*.py' from .gitignore
|
Hi @matprov I have made your requested changes (just let me know if issues still remain). Sorry for delay on this. |
|
Hi @JeremyFyke The new version is deployed. I would suggest changing the image of the new hazards panel. It's a bit confusing for users to always see new image for panels but this image is used twice. |
|
@matprov this is great, thanks. One thing I noticed, is that on final summary stage, the accordion-style expandable list of resources expands below the range of the page. Seeing these is important (in fact, this is the primary outcome of the tool). Could we make this web page adaptively sized, so the height of the page grows as needed? Or alternatively, for the moment, just expand the static height of the html container so it definitely captures reasonable expansions of this accordion tool? A long term solution could be to make this accordion tool a separate tab entirely, so it doesn't 'stack' below the SanKey diagram. On a general note, aside from pull request review work, how much work is it for you to do these deployments to web? Is it like 10 minutes, or 2 hours? Just curious, to know how much work we are asking of you each time we request a new deployment! |
|
Hi @JeremyFyke Yes, the layout will be fixed in order to show all the content from the accordion-style view. It will be based on the content, so the page will grow when accordion is expanded. Deployments takes couple of minutes. It's only a matter of building locally the docker image to make sure the app itself works, then pushing changes in staging. I prefer to have multiple smaller deploys than waiting for bigger ones with bigger changes. |
Hi @matprov OK thanks. This is great, can you let me know when the 'page growing' option is enabled? This should be last thing I need before I show this tool to some important building sector experts for review. Related: I would also be happier with multiple smaller deploys as well (so feedback from these reviews is fairly rapidly available in the updated development site app). |
Hi @matprov - could I request a pull request into main, and then deployment of a new tag to climatedata.crim.ca? I'd like to demo this version to a few parties in the next few days, if I can.
Note, I disabled pre-commit on merges to dst-core from individual sandbox branches. Is there a way to just run pre-commit on merges to main? I definitely value pre-commit but it's REALLY hard to manage pre-commit with new folks who are very new to software and don't understand git flow very well. If you/I could just do pre-commit runs on less-frequent merges to main that would be super helpful on my end. Your thoughts welcome.
Jeremy