Esri / distance-direction-addin-dotnet

Add-in provides the ability to easily and quickly create geodesy lines, circles, ellipses and range rings.
Apache License 2.0
17 stars 22 forks source link

Comma causes error message Range Rings tab in Distance and Direction but not on Circle tab #514

Closed BobBooth closed 6 years ago

BobBooth commented 6 years ago

The logic used to parse input values on the Circle tab should also be used on the Rings tab for the Origin and Cumulative construction methods. Decimal values work, but using a comma as a thousands separator causes the tool to give an "Ensure all distances are numeric" message. image

@adgiles @kgonzago

BobBooth commented 6 years ago

@adgiles - maybe this could be fixed before @kgonzago PR gets merged into core?

adgiles commented 6 years ago

With the changes I am currently making to the workflow you will be unable to put a comma into the box

kgonzago commented 6 years ago

FYI, PR was merged.

BobBooth commented 6 years ago

@adgiles - If commas work on the Circle tab, why not on this one? What about places where the comma as a decimal separator instead of a period?

adgiles commented 6 years ago

the distances for the range values are now only going to accept numeric values, just realised that you may want to use decimal points and commas so may need to have a look at what i have done

kgonzago commented 6 years ago

Changes are now in a new PR created in Devtopia.

@BobBooth Can you test this branch

BobBooth commented 6 years ago

@kgonzago - wait, did something wrong.

adgiles commented 6 years ago

@bobbooth @kgonzago I’m going to have to look at this a bit more, the issue is the simple table dijit only accepts text as entries where we are using the number validation on other inputs. We now have to do the validation ourselves.

It’s not just a case of removing commas from the inputs as in some languages comma’s are used as decimal points. My recommendation would be only to accept one comma or decimal point to be used to represent a decimal point value

BobBooth commented 6 years ago

OK, so if I set the distance values to 4,000 meters and 5,000 meters, the tool does run, but it interprets the comma as a decimal point. I get rings with 4.0 and 5.0 meter circles. image

If I set it to 33,000.5 - that is, a comma and a decimal point, then I get the "must be numeric" warning. image

@kgonzago

adgiles commented 6 years ago

@bobbooth yes at the moment it is thinking the comma is a decimal point, I need to do more work on this tomorrow

BobBooth commented 6 years ago

The circle tool handles this case. image Can we not pass the numbers from the table through the same validation code used by the circle tool?

adgiles commented 6 years ago

No, they are using different components of the dojo framework

adgiles commented 6 years ago

@bobbooth I have refactored the code i was using and managed to get the input boxes within the table to use the same input type as used within the circle tool. Once @kgonzago has pushed the changes up to devtopia you should be good to test.

kgonzago commented 6 years ago

FYI: Changes were pushed to this PR. This branch can be used for testing.

BobBooth commented 6 years ago

@adgiles @kgonzago Tested this, getting error on launch widget: image

Unfortunately, I'm heading out now for the weekend. See you Monday!

ACueva commented 6 years ago

@kgonzago let's chat regarding code differences between internal and external GitHub repos.

kgonzago commented 6 years ago

The widget didn't launch due to an additional semicolon. Fixed it in devtopia.

The widget in devtopia and this repo are now sync'ed using this branch. The branch will not be merged with dev branch anytime soon.

BobBooth commented 6 years ago

Commas, decimals, and mixed commas & decimals is working with this fix, in branch @kgonzago mentioned (https://github.com/Esri/distance-direction-addin-dotnet/tree/kg/wab-dev). See image: image.png

dfoll commented 6 years ago

I looked at this as well and everything looks ok with the commas and decimals

lfunkhouser commented 6 years ago

@BobBooth @dfoll Was the checklist test updated to test the variations?

BobBooth commented 6 years ago

@lfunkhouser - I had a commit in one of the PRs that somehow got lost in the merging process. Have re-added that info and expanded. Asking @dfoll to merge. https://github.com/ArcGIS/solutions-defense-test-catalog/pull/94

adgiles commented 6 years ago

@topowright @dfoll @ACueva

now showing in the daily build (01 Mar 18), good to test

dfoll commented 6 years ago

-verified fixed with the daily -verified @BobBooth updates to the checklist test to instruct the user to user commas and decimal points

If this needs tested explicitly on devext we need to move this back. Was not able to text on devext because I could not log in because I could not figure out what the password is.