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

Feature: NumberBox (dev tracking) #1505

Closed
23 of 27 tasks
teaP opened this issue Oct 29, 2019 · 12 comments
Closed
23 of 27 tasks

Feature: NumberBox (dev tracking) #1505

teaP opened this issue Oct 29, 2019 · 12 comments
Assignees
Labels
area-TextBox TextBox, RichEditBox team-Controls Issue for the Controls team working on it

Comments

@teaP
Copy link
Contributor

teaP commented Oct 29, 2019

Tracking for #483

  • Initial checkin after preliminary cleanup
  • Reimplement AcceptsCalculations et. al. Some notes for the current implementation:
    • Negative not supported in front of parens, e.g. "2 * -(3 + 2)"
    • (3) doesn't evaluate to 3.
    • -2 - 5 doesn't evaluate to -7.
    • 5.09 + 14.333 = 19.423000000000002, why
    • . needs to be globalized
    • What is ComputePrecisionRounderSigDigits et. al. doing?
  • Additional features
    • Add flyout spin buttons
    • Treat NaN as an "unset" value, so the Text becomes an empty string.
  • Bug fixes
    • Doesn't evaluate when Text property changes
    • Minimum can be larger than maximum, and vice versa
    • Doesn't strip whitespace (so "5 " is invalid)
    • When an invalid number is set as the value, we currently are sending ValueChanged twice.
    • If a string/expression evaluates the current value, the string isn't replaced, but it should be.
    • Could use a test for . vs ,
    • Header should be an object; also add HeaderTemplate.
    • When control gets focus it should select text. On evaluation, the caret should be moved to the end of the value.
    • Spin buttons should gray out when min/max is hit (if validation is not disabled)
    • What happens if you type in a number (or expression!) and hit the up/down buttons before it's evaluated...?
    • Setting Text initially in markup doesn't do anything.
    • Arrow keys only work on key up, so long press doesn't do anything (there's no repeat)
  • Spec questions
    • How should the gamepad work -- should d-pad up/down increase/decrease the value?
    • Shift-up/down does larger change? (Need additional property for this?)
@teaP teaP added working on it area-TextBox TextBox, RichEditBox labels Oct 29, 2019
@teaP teaP self-assigned this Oct 29, 2019
@msft-github-bot msft-github-bot added this to Needs triage in Controls Triage Oct 29, 2019
@jevansaks jevansaks moved this from Needs triage to Backlog in Controls Triage Oct 30, 2019
@jevansaks jevansaks added the team-Controls Issue for the Controls team label Nov 7, 2019
@msft-github-bot msft-github-bot moved this from Backlog to Needs triage in Controls Triage Nov 7, 2019
@msft-github-bot msft-github-bot added the needs-triage Issue needs to be triaged by the area owners label Nov 7, 2019
@jevansaks jevansaks removed the needs-triage Issue needs to be triaged by the area owners label Nov 7, 2019
@harvinders
Copy link

@teaP I am not sure if raising a issue is the best way to give feedback on something that is still under construction. I am using the latest preview and found following.

  • Setting numberBox.Header = null throws exception. Says, can't be set to null. However, for other controls like ComboBox , TextBox etc we are able to set it to null.
  • Also, could you please add support for HeaderTemplate, All other similar controls have it.

@teaP
Copy link
Contributor Author

teaP commented Nov 13, 2019

@teaP Tammy Paine FTE I am not sure if raising a issue is the best way to give feedback on something that is still under construction. I am using the latest preview and found following.

  • Setting numberBox.Header = null throws exception. Says, can't be set to null. However, for other controls like ComboBox , TextBox etc we are able to set it to null.
  • Also, could you please add support for HeaderTemplate, All other similar controls have it.

Thank you, @harvinders, for bringing this to my attention! Header was defined as a string for some reason, it's an object now like all the others. I've also added HeaderTemplate. The fixes will be in my next checkin, hopefully early next week.

@adrientetar
Copy link

adrientetar commented Nov 18, 2019

Could ValueChanged only be sent when the user changed the value and not when programmatically setting it? Right now, I have to exclude that case manually cc @SavoySchuler @MikeHillberg

Other feedback from the latest pre-release package:

  • The value should be committed on loss of focus (if I change value and click somewhere else)
  • The control doesn't seem to update itself when disabled, here I set value to NaN and which disables the control (I use a doubletovisibility converter in the xaml for that) it becomes disabled but still shows the previous value (421.5), it should show NaN
    image
  • On long press on an arrow key, it should change the value quickly (~inertia)

Feedback taken from my previous comment:

  • I need a way to display nothing, in the case where no value ought to be displayed in the control. With TextBox I just set string.Empty but with NumberBox.Value there is no way to do that. I propose that double.NaN be equivalent to string.Empty for the TextBox, and display nothing (currently shows NaN). I see NumberBox has a Text property that I could also set but I don't want to "play around" with both Text and Value that are related properties.
  • Should SelectAll when the control gets focus and also when using arrows to change the value
  • Shift+Up/Down to add in increments of 10 isn't there, an important part to me. Could also Ctrl+Shift for increments of 100.
  • The click+drag up/down to change the value would be nice to have.

@teaP
Copy link
Contributor Author

teaP commented Nov 19, 2019

Hi @adrientetar, thanks for your feedback! In the prerelease control, the value should be committed on loss of focus -- is that not working for you? I also couldn't repro the disabled issue.

I've added the long press arrow key feature to my todo list. My next update will also treat NaN as an empty value. I'll start a discussion about Shift+up/down and get back to you on that one.

@adrientetar
Copy link

adrientetar commented Nov 20, 2019

@teaP, actually yeah, the value is committed on loss of focus. My issue is that if I click on my app canvas it will deselect the items before the NumberBox has a chance to commit, because I guess focus lost event isn't sent out immediately but needs wait for the event loop to return.

Video. The code change is here, also you can see I need a _isEditing boolean value to filter ValueChanged when it comes from the user, it would be nice if the control can do that internally maybe (or add a Reason property to the arguments perhaps to say if it's User change or Programmatic).

For the disabled issue, I use a converter on the value to set IsEnabled:

image

@teaP
Copy link
Contributor Author

teaP commented Nov 21, 2019

@adrientetar, hmm, would it help if you had a way to force validation of the text before you deselected the item in your canvas?

@adrientetar
Copy link

@teaP, not really because that would introduce too much coupling between parts of my app. I think I just need to do something like call CoreDispatcher.ProcessEvents so that the focus out event is processed immediately and before I deselect. So I don't think anything is required on the control, that's just something I thought I would note.

@adrientetar
Copy link

adrientetar commented Nov 27, 2019

@teaP, it seems the NumberBox introduces 2 elements in the visual tree that receive focus

image

Here I see NumberBox get focus when tabbing and then InputBox as well. Shouldn't only NumberBox be in the tab focus order?

Also:

  • When I click into the NumberBox and hit Up for the first time, it moves the cursor to the left instead of stepping up the value.
  • The SelectAll() when clicking into (focusing) NumberBox should happen on mouse up, not mouse down I think. If it happen on mouse down you can still potentially unselect part of the text before mouse up.

@jevansaks
Copy link
Member

Thanks for letting us know @adrientetar, do you mind opening this as a new issue just so we don't lose it?

@jevansaks
Copy link
Member

  • The SelectAll() when clicking into (focusing) NumberBox should happen on mouse up, not mouse down I think. If it happen on mouse down you can still potentially unselect part of the text before mouse up.

That is standard TextBox behavior and not something NumberBox is doing. I think it might be intentional. You might have seen it because you were trying the "hyper scrolling" feature which we ended up cutting from the 2.3 release.

Here I see NumberBox get focus when tabbing and then InputBox as well. Shouldn't only NumberBox be in the tab focus order?

I tried this in the test app and I don't see this. Only one thing is getting visited by Tab when I tab through the page. Can you share a repro app?

  • When I click into the NumberBox and hit Up for the first time, it moves the cursor to the left instead of stepping up the value.

I see this, it's because TextBox eats that first Up to move the caret. We can try to get in the way and handle it, so go ahead and file an issue for this.

@mdtauk
Copy link
Contributor

mdtauk commented Nov 27, 2019

Assuming the default CornerRadius is set properly, here is the current styling in Light and Dark themes, as well as with different backgrounds

image

The compact flyout is visibly lost in the Dark Theme - using Acrylic for this flyout would be one way to solve it, as well as adding borders to these flyout buttons.

@teaP
Copy link
Contributor Author

teaP commented Dec 9, 2019

V1 finished, additional comments should go to #1736,

@teaP teaP closed this as completed Dec 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-TextBox TextBox, RichEditBox team-Controls Issue for the Controls team working on it
Projects
No open projects
Controls Triage
Needs triage
Development

No branches or pull requests

6 participants