-
Notifications
You must be signed in to change notification settings - Fork 621
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
[DNM] Align node previews with number format selected in preferences #13076
base: master
Are you sure you want to change the base?
Conversation
@@ -208,6 +208,8 @@ internal static object GetData(StackValue sv, RuntimeCore runtimeCore) | |||
return null; | |||
} | |||
|
|||
internal static string PrecisionFormat { get; set; } = "f3"; |
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.
Same as default value in preferencesettings.numberformat
.
@@ -1574,7 +1574,7 @@ private IPreferences CreateOrLoadPreferences(IPreferences preferences) | |||
|
|||
private static void InitializePreferences(IPreferences preferences) | |||
{ | |||
DynamoUnits.Display.PrecisionFormat = preferences.NumberFormat; |
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.
What happened to DynamoUnits.Display.PrecisionFormat
?
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.
It's still there in the next line.
It looks good @aparajit-pratap - maybe we can add a simple test though using one of the customer graphs that prompted this change? |
Let's wait for a decision from @Amoursol, @Jingyi-Wen, and the rest of the team on the next steps for this. For this PR to be given the green light some of the questions that need answering/assumptions made are:
|
@aparajit-pratap thank you for putting this PR in! My answers as below:
|
My thoughts:
|
Regarding "We may need another Preference setting here that enables rounding under a certain threshold for users", I believe this is a bigger task that would need addressing at a core level that would affect all numeric results, certainly not in this PR. Until then I suppose we can either wait until those changes happen or first roll out this part where we are only using preferences for display purposes. @Amoursol @Jingyi-Wen any thoughts about the ☝️ suggestion? |
Looks good to me @aparajit-pratap - We can look to further rounding improvement later. |
Closing since this is now duplicating #13744 |
Purpose
See slack thread.
Declarations
Check these if you believe they are true
*.resx
filesRelease Notes
(FILL ME IN) Brief description of the fix / enhancement. Mandatory section
FYIs
@Amoursol