Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug/319 localization double parse #349

Merged
merged 8 commits into from
Aug 1, 2024

Conversation

TimPurdum
Copy link
Collaborator

Closes #319

  • Passes navigator.language with graphic attributes in dotNetBuilder.buildDotNetGraphic
  • Uses the passed culture string if available to parse numbers and datetimes. Falls back to CurrentCulture.
  • Added new unit tests to validate parsing
  • Added support in TestRunnerBase.razor for synchronous tests

There is some question in my mind still if this is the "right" or "best" approach. The issue IMO stems from the following:

  • Data sources, such as AGOL, can store feature data with a culture
  • Client devices can have a different culture
  • GeoBlazor servers could in theory have yet another culture

How these 3 pieces fit together, and how to know that we aren't breaking data in any one scenario (data from AGOL in culture A, data from GeoBlazor in culture B, etc.).

For example, if we pass data to ArcGIS JS layers in culture A from GeoBlazor, but the client is in culture B, when it returns to GB, it will have culture B tied to the data. Some of this stems from the fact that Blazor is both a server and client technology, often in the same application.

@TimPurdum TimPurdum self-assigned this Jul 21, 2024
@TimPurdum TimPurdum marked this pull request as ready for review July 28, 2024 16:02
@TimPurdum
Copy link
Collaborator Author

Questions above aside, these changes will make GeoBlazor work inline with the ArcGIS JS SDK, along with a few other options that you could use to manipulate the AttributesDictionary server-side. I think that is a step in the right direction, it fixes the issue raised by an end user in Europe, and it should be merged.

public AttributesDictionary(Dictionary<string, object?> dictionary)
{
CultureInfo cultureInfo = dictionary.TryGetValue("geoBlazorCulture", out object? gbCulture)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to make this a property on the class rather then another entry in the dictionary? It could help separate data vs framework, unless these attributes are not directly realted to the data.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is the serialization of the attributes, where we have to switch them to an array of objects with key, value, and valueType. I could rewrite all of that to be an object with an array, but this seemed simpler. Rewriting would mean rewriting both JSON and Protobuf serialization methods in both C# and TypeScript. 😩

@TimPurdum TimPurdum requested a review from AndersenBell July 31, 2024 23:53
@TimPurdum
Copy link
Collaborator Author

After discussing with @AndersenBell, we came up with a better plan. In my changes, we now query once for the browser language in JsModuleManager when the js modules are loaded. This is then referenced in AttributesDictionary to deserialize, and we no longer need to add the culture to each attribute dictionary.

Copy link
Collaborator

@AndersenBell AndersenBell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good

@TimPurdum TimPurdum merged commit 753a22f into develop Aug 1, 2024
1 check passed
@TimPurdum TimPurdum deleted the bug/319-localization-double-parse branch August 1, 2024 13:44
@seahro seahro mentioned this pull request Aug 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Localization Issue with comma decimal and serialization
2 participants