Entry form #143
Entry form #143
Conversation
jbkolze
left a comment
jbkolze
left a comment
There was a problem hiding this comment.
in general, I feel like we might be reinventing the wheel a little bit with all of the form state/registration/validation code. This is mostly a solved problem with libraries like react-hook-form.
While I don't want to add a lot of dependencies to GWW, in my opinion if we're going to support forms to this level then it would make sense to lean on an established framework rather than rolling our own. That would cut down drastically on a lot of the boilerplate code we have here for managing/registering inputs and mostly leave the CWMS-specific logic, which is really the part we want/need to handle ourselves.
krowvin
left a comment
krowvin
left a comment
There was a problem hiding this comment.
Correct all instances of water.usace to cwms-data.usace
| {`import { CWMSDropdown } from "@usace-watermanagement/groundwork-water"; | ||
| import { FormWrapper } from "@usace-watermanagement/groundwork-water"; | ||
| <FormWrapper office="SWT" cdaUrl="https://water.usace.army.mil/cwms-data"> |
There was a problem hiding this comment.
All references to water.usace need to be changed to cwms-data.usace
The reason is the reverse proxy to water.usace has been removed now that we've gone to cloud.
It might come back.
|
Sorry for the wait on this, @oskarhurst . I keep getting tied up with other things. I'm shooting to get it reviewed for you on Friday. |
jbkolze
left a comment
jbkolze
left a comment
There was a problem hiding this comment.
Raw CSS is still used for styling in the majority of the input components. The style styling needs to be converted to tailwind className for consistency with the rest of the library.
As a general note, a single commit of 10,000+ lines is very difficult to review due to the massive amount of code changed in one context. Please try to break up commits where possible, e.g. for a single feature, for a particular fix, in response to a specific comment, etc. That'll make our lives a lot easier on the other end of things. Thanks!
There was a problem hiding this comment.
Input component clears on submit -- I wonder if there might be cases where this isn't desired? Possibly worth adding a toggle?
| <FormWrapper office="SWT"> | ||
| <CWMSForm office="SWT"> | ||
| <CWMSInputTable | ||
| tsids={["Elev.Inst.0.0.USGS", "Storage.Inst.0.0.USGS"]} |
There was a problem hiding this comment.
Are these valid timeseries IDs? I know this is an example but I would still provide a complete TS idea so it's clear.
I considered maybe this somehow already gets the location but I wouldn't expect that myself! And would want users to be explicit.
I.e.
KEYS.Elev.Inst.1Hour.0.Ccp-Rev. The current tsids appear to be missing the location.
| defaultValues={{ | ||
| "Elev.Inst.0.0.USGS_-3600": "650.5", | ||
| "Elev.Inst.0.0.USGS_0": "651.2", | ||
| "Storage.Inst.0.0.USGS_0": "125000", |
docs/src/pages/docs/forms/index.jsx
Outdated
| CWMS input components for data collection. The FormWrapper handles all the CWMS integration | ||
| Start with the{" "} | ||
| <a | ||
| href="/docs/forms/cwms-form" |
There was a problem hiding this comment.
I think this is going to cause an issue with routing.
I apologize for
- This PR is not in yet
- It's not well documented
But we must include BASE_URL in front of all paths. Otherwise when a user clicks this it will not know where to go.
At the top of the file(s)
const BASE_URL = import.meta.env.BASE_URL;
This pulls it from the global vite.config.js
Then in the href(s) you can:
{`${BASE_URL}/#/docs/react-query`}
This is just to keep it in line with the future PR. They work on localhost, but that's the client-side routing doing it's magic. A ctrl + click on github once deployed will fail.
docs/src/pages/docs/forms/index.jsx
Outdated
| <li><strong>CWMSSpreadsheet</strong> - Excel-like spreadsheet with copy/paste support</li> | ||
| <li> | ||
| <a | ||
| href="/docs/forms/cwms-form" |
docs/src/pages/docs/forms/index.jsx
Outdated
| </li> | ||
| <li> | ||
| <a | ||
| href="/docs/forms/cwms-input" |
docs/src/pages/docs/forms/index.jsx
Outdated
| </li> | ||
| <li> | ||
| <a | ||
| href="/docs/forms/cwms-textarea" |
docs/src/pages/docs/forms/index.jsx
Outdated
| </li> | ||
| <li> | ||
| <a | ||
| href="/docs/forms/cwms-checkboxes" |
docs/src/pages/docs/forms/index.jsx
Outdated
| </li> | ||
| <li> | ||
| <a | ||
| href="/docs/forms/cwms-dropdown" |
docs/src/pages/docs/forms/index.jsx
Outdated
| </li> | ||
| <li> | ||
| <a | ||
| href="/docs/forms/cwms-input-table" |
docs/src/pages/docs/forms/index.jsx
Outdated
| </li> | ||
| <li> | ||
| <a | ||
| href="/docs/forms/cwms-spreadsheet" |
| return ( | ||
| <> | ||
| <Component {...props} /> | ||
| <EnsureToastContainer /> |
There was a problem hiding this comment.
I'm surprised that the only way to know if this has been created in the DOM is to check with a querySelector above.
If I understand correctly, you've wrapped the check with the component to prevent it from being added if a user adds it elsewhere?
Does this logic to prevent duplicates not already exist within the ToastContainer?
There was a problem hiding this comment.
To clarify, i'm asking having not googled or researched this. Just thinking out load about how you had to do this but I would hope Toastify handles this because it's common for an app to accidentally create multiple toasts.
There was a problem hiding this comment.
@krowvin after more research now i see that toast contains has an enablemulticontainers argument which i can use instead. originally I was just thinking what is the fastest whay to see if one exists and if so dont make a new one, but this is not needed.
|
Sorry, one more thing -- I think you need to commit the updated package-lock.json file to match the package.json changes. |
|
Pending you can resolve the issues @jbkolze found lets move to merging this in. May also want to consider presenting at the web workshop? |
|
We have an hour time slot for online forms at next weeks in-person workshop. Troy will be sharing how he is using/testing this product. @jbkolze your welcome to add to this conversation. The plan is to have these presentations available virtually. Thanks again for all of your feedback on this product. |
krowvin
left a comment
krowvin
left a comment
There was a problem hiding this comment.
Previous revisions I had look good!
|
Hi sorry for resolving the comments in the past two weeks I have been out of leave. @krowvin I was trying to implement the const BASE_URL = import.meta.env.BASE_URL; for the hefs but I could not get them to work it would always append and extra /#/ to what ever link I was tryign to use any recommentation on how to fix this. @jbkolze I was finally able to find the thing cause only the last row to be save the inputs cell would over write each other in the CWMS form so that should be resolved now. |
jbkolze
left a comment
jbkolze
left a comment
There was a problem hiding this comment.
Sorry for the delay on this, @oskarhurst -- was taking some leave myself.
Updates look good to me. Thanks for breaking up the commits a little more! Made it a lot easier to parse through the edits.
There's just a little bit of package.json stuff that needs to get cleaned up before merge.
docs/src/config/docs.config.js
Outdated
| features: { | ||
| // Toggle visibility of the interactive test page | ||
| // Set to true to show in navigation, false to hide | ||
| showInteractiveTestPage: true, |
There was a problem hiding this comment.
Do we want to show this by default (i.e. include in GH-docs) or just provide the toggle so devs can turn on for local use?
There was a problem hiding this comment.
Sorry that is my mistake I forgot to set that to false.
| { | ||
| "name": "@usace-watermanagement/groundwork-water", | ||
| "version": "3.4.1", | ||
| "license": "MIT", |
There was a problem hiding this comment.
Missing a comma after the watch script
There was a problem hiding this comment.
This is out-of-date again after merging the main branch back in. Just need to npm install and commit.
@krowvin Sounds like you might want to touch base with @oskarhurst on the BASE_URL stuff? |
|
I'm thinking we go ahead and merge in what Oskar has now. He updated the links to work. Once he merges I should be able to update my PR, do any formatting I need, and merge with his changes (not needing to go back and fix any links he has too). Keeping my PR approved! |
jbkolze
left a comment
jbkolze
left a comment
There was a problem hiding this comment.
Looks good. Thanks for all your work on this, @oskarhurst!
5 participants
Initial Testing PR for Ground work CDA data entry form. [DO NOT MERGE]
Feel Free to open the docs site for this branch to get a better idea for all the componets. More testing needs to be done to make sure it works correctly, but I made a built in testing tool in the docs so you can connect to a cda instance and test and play around straight from the docs if that would be useful. Components under active development.