WM Data Hooks - Githubissues

USACE / groundwork

React Components for USACE Applications

https://usace.github.io/groundwork

4 stars 0 forks source link

WM Data Hooks #15

Closed jbkolze closed 2 months ago

jbkolze commented 3 months ago

Initial draft for incorporating hooks into the library. Focused on hooks for interfacing with CDA for this first run. A few specific items of note:

As discussed a while ago, included typescript compilation to parse the hooks since they were built in TS originally. Didn't notice any issues with this process, but could've missed something. The typing is pretty minimal so it'll be easy for me to convert these to JS and rip out TS if this causes issues (or if it's a bad idea for any other reason).
There's a CDA_URL constant included that is used as the default CDA instance (currently pointed at the CWBI-test instance), but I often need to change this myself for development (and I imagine other developers might need to as well). Currently including an optional "cdaUrl" parameter in the CDA hooks, but this feels a little clunky and seems like it might be better handled as some sort of config file. Thoughts?
The current structure feels a little weird (WM -> Data Hooks / useTS / useLocation / etc...). Can throw these under a "Hooks" section, but they also seem pretty WM-specific. Good case for nested sections?

jbkolze commented 3 months ago

Converted to a draft request so this doesn't accidentally get merged before it's ready since there are some somewhat big changes (i.e. typescript).

willbreitkreutz commented 2 months ago

Hate to do this to you @jbkolze, but you'll want to pull main into this branch to get all the class name updates that i did, and that will mean that any tailwind classes you're using will also need to be prepended with gw- since we're now prefixing any css that comes with groundwork.

jbkolze commented 2 months ago

No problem, @willbreitkreutz -- just to check, any preference on rebasing vs. merging for pull request updates moving forward? I generally like to rebase where possible just for a cleaner history, but know that could cause issues if multiple people are working on the branch.

willbreitkreutz commented 2 months ago

I was just thinking about that yesterday... I think typically we'll be working on individual branches so i'm ok with rebasing, if it starts to cause issues we can reassess

jbkolze commented 2 months ago

Should be ready to go now, @willbreitkreutz . I can only imagine how fun updating every gw- classname in the repo was...

jbkolze commented 2 months ago

As I'm moving on to other TSX components I'm seeing that typescript is going to want type definitions for any existing JSX components that are used (e.g. Card) since it assumes properties are required by default. Creating the .d.ts files is pretty easy, but they'll need to be updated/maintained if future edits are made, and if I'm the only doing it (and, of course, the only one using the benefits) then it's probably just going to make the codebase messier with very little gain if full typescript isn't an objective.

That being the case, I think I'm just going to bite the bullet and rip out all of the typescript from this request and maintain my solitary bastion of strict typing in the LRL repo. I'll hopefully have the branch updated shortly assuming I don't run into any unforeseen hurdles.

jbkolze commented 2 months ago

TypeScript officially blown away. We hardly knew ye.

willbreitkreutz commented 2 months ago

@jbkolze that took guts

jbkolze commented 2 months ago

@willbreitkreutz I think this is (finally) ready to be merged pending your approval.

I looked into using the react-query default function and I think it would make things pretty messy (especially for any users who are using more than the base hooks we provide) so I just threw in the global variable for now. Can discuss further if you see that being a problem. Ideally we come up with a better configuration solution down the line, but hopefully that will suffice for the time being.