USACE / water-ui-old

Access to Water User Interface (UI)
0 stars 0 forks source link

Feature/187 d3 dam profile #208

Closed dlin2102 closed 3 years ago

dlin2102 commented 3 years ago

Initial d3 dam profile with dummy data. I wasn't sure which property/bundle to use so I left it as dummy data for now. There are 4 modes. dam, lock, turbine, and lockTurbine. You can change the values on lines 1100 - 1102 in DamProfileChart to see each version. Let me know what you guys think.

brian428 commented 3 years ago

@dlin2102 I pushed some updates to use the values from the location detail. One problem I noticed is that the D3 logic seems to be appending duplicate SVG elements each time something does an .append(). For example, I was not seeing the labels on the circles for IN, OUT and SUR. I modified those functions to try to separate the initial append from the subsequent appends and that seems to have gotten the labels to display. But if you look at the Chrome elements tab, within the D3 chart you'll see a lot of duplicated items with the same class names. Can you look into why this is happening? It seems odd that D3 would just append a duplicate after you select something, but that's what it seems to be doing. Maybe there's something wrong with the way the old (and new) logic is building up the diagram. If you also see this on your side, see if you can figure out what's going on.

brian428 commented 3 years ago

@dlin2102 I should add that I also looked at the old site code and added in some of the other params it seems to use for the dam profile. And also some logic to show the other lines and labels if they are available. You'll see this in the code that builds up the data to pass into the DamProfileChart.

brian428 commented 3 years ago

@dlin2102 and finally, there is some logic in the old code (at a2w-site\src\js\services\a2w.data.BasinStreamService.js) that seems to build up some gradients for showing flood conditions. I think you can see this gradient if you open up the full report at https://water.usace.army.mil/a2w/f?p=105:1:::::P1_LINK:4008030-CWMS-report. I think for now this is commented out of our version, but see if you can get that working as well if you can. Thanks.

dlin2102 commented 3 years ago

@brian428 That's weird. I do see the labels on my local and don't see any duplications elements in the d3. Unless, I'm missing something? image image

brian428 commented 3 years ago

hmm interesting. Have you pulled my updates in? I wonder if it is something caused by using the "real" data somehow. Let me know.

dlin2102 commented 3 years ago

No, not yet. I'll pull in your changes and try to figure it out.

mohitb3 commented 3 years ago

@brian428 yea I pulled your changes and I saw the duplicate svg elements as well. I went one commit back and the duplicate elements were removed. It probably has to do with the changes that were made when adding the real data

brian428 commented 3 years ago

@dlin2102 ok, seems odd though. If you look at my changes to the actual D3 chart, mostly they were just trying to fix this dupe issue (by separating some of the append() calls) and changing sur to surcharge for clarity. See what you can figure out! Thanks.

brian428 commented 3 years ago

Unless, just an idea...but now that it is actually using the "real" data, the selector data changing may be trigger something to run more than once (via useEffect()), so maybe if the effect is triggered more than once is triggering more than one run to build up the D3 element? Hopefully I'm making sense. No idea if this could be part of the problem but just something I thought of.

dlin2102 commented 3 years ago

Yeah, ur right. Once I made it onMount, there was no duplication.

brian428 commented 3 years ago

Very nice. Dan, should I go ahead and merge this? Or are you still taking a shot at getting that flood gradient to work? If you we could add a separate ticket for that, unless you're already in the middle of doing that.

dlin2102 commented 3 years ago

I'm currently working on the gradient but we could just add another ticket for that.

brian428 commented 3 years ago

ok I'll go ahead and merge this!

brian428 commented 3 years ago

Actually you can just push to this branch and I'll just merge it again. Probably no need for another ticket and branch unless you really want to.