-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
#16271: Fixing that saving workflow fails when using decimal comma (,) #16274
Conversation
@@ -475,8 +476,8 @@ public async Task<IActionResult> Edit(WorkflowTypeUpdateModel model) | |||
foreach (var activityState in activities) | |||
{ | |||
var activity = currentActivities[activityState["id"].ToString()]; | |||
activity.X = (int)Convert.ToDecimal(activityState["x"].ToString()); | |||
activity.Y = (int)Convert.ToDecimal(activityState["y"].ToString()); | |||
activity.X = (int)Convert.ToDecimal(activityState["x"].ToString(), CultureInfo.InvariantCulture); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure why we need a decimal while at the end we convert the values to integers.
/cc @hyzx86
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure, maybe the X, Y attributes are defined as Integer so they need a layer of conversion, I wouldn't mind changing them directly to Decimal
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's better to keep X, Y attributes as int. Most browsers truncate the fractional part if the position is set with decimal numbers (10.75px -> 10px).
I've made a version where the state is returned from the client without the fractional part. It uses "Math.trunc" that is only available from javascript version es6/es2015.
src/OrchardCore.Modules/OrchardCore.Workflows/Controllers/WorkflowTypeController.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm also OK with this but please reply under https://github.com/OrchardCMS/OrchardCore/pull/16274/files#r1633259224.
src/OrchardCore.Modules/OrchardCore.Workflows/Controllers/WorkflowTypeController.cs
Outdated
Show resolved
Hide resolved
…16205) Co-authored-by: Mike Alhayek <[email protected]> Co-authored-by: Hisham Bin Ateya <[email protected]>
fb84bbc
to
551008b
Compare
This pull request has merge conflicts. Please resolve those before requesting a review. |
# Conflicts: # src/docs/resources/libraries/README.md
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works fine too. If anybody else would like to check this, please do, or I'll merge it tomorrow.
Fixes: #16271