Fixes to a few NumberBox issues #1894
Conversation
msft-github-bot
added
the
needs-triage
ranjeshj
added
needs-cherrypicktorelease
ranjeshj
added
area-NumberBox
dev/NumberBox/NumberBox.cpp
Outdated
| // This was largely copied from Calculator's GetRegionalSettingsAwareDecimalFormatter() | ||
| winrt::DecimalFormatter NumberBox::GetRegionalSettingsAwareDecimalFormatter() | ||
| { | ||
| std::vector<winrt::hstring> languageList; |
There was a problem hiding this comment.
std::vectorwinrt::hstring languageList; [](start = 4, length = 41)
Why is this a vector? seems like we only ever add one item max
There was a problem hiding this comment.
That's just what the constructor for NumberFormatter wants.
There was a problem hiding this comment.
Maybe it would be cleaner to create the vector only once you have a language name? You might be able to do it with initializer syntax in the ternary operator below, I'm not sure.
In reply to: 372026234 [](ancestors = 372026234)
There was a problem hiding this comment.
Maybe initialize the size to 0? I'm not sure if that avoids an allocation. Maybe just look at / debug through the STL implementation? https://github.com/microsoft/STL/blob/master/stl/inc/vector#L469
There was a problem hiding this comment.
I don't think I can do it in one line so I switched it around a bit.
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
If you look closely at the up arrow then you can see that there are still scaling issues there. It would be great if this could be fixed before the pull request is merged. |
Yeah, I know, but unfortunately it's a CornerRadius issue. Having any value >0 in CornerRadius scales the border differently than if it's 0.
I followed up offline with the team that owns rendering, but for this PR there's nothing more I can do. |
|
|
||
| if (winrt::Language::IsWellFormed(currentLocale)) | ||
| { | ||
| languageList.push_back(winrt::hstring(currentLocale)); |
There was a problem hiding this comment.
you could use emplace_back(currentLocale);
| if (!formatter) | ||
| { | ||
| formatter = winrt::DecimalFormatter(); | ||
| } |
There was a problem hiding this comment.
I'd probably make this a lambda to avoid the scratch variable.
* Fixes to a few NumberBox issues. * PR update * PR feedback and fixed downlevel missing resource.
* Fixes to a few NumberBox issues. * PR update * PR feedback and fixed downlevel missing resource.
ranjeshj
removed
the
needs-cherrypicktorelease
I originally assumed that creating a new DecimalFormatter would, by default, conform to the user's selected language/region. Confoundingly, it does not. I lifted the related code in Calculator to create a DecimalFormatter with the right defaults, which fixes #1798.
I also moved the header/description content from being handled by EditBox to being in NumberBox directly. This allows me to size the up/down buttons to be the same size as the EditBox, which fixes #1846 and fixes #1839.
With description text at 125% DPI:
