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

Add variable editing #581

Conversation

joseartrivera
Copy link
Contributor

Description of the changes:

As part of #338, this PR adds the UI and backend for editing variables that exist for each equation. Variables can be modified with a slider and textbox. Min/Max/Step can be modified by selecting the gear icon for each variable (temporary icon).

The entry point to the UI is temporary as that is still being finalized.

How changes were validated:

Manual testing

@joseartrivera joseartrivera changed the title Joriv/variables Add variable editing Jul 10, 2019
src/Calculator/App.xaml Outdated Show resolved Hide resolved
t get() { return m_##n; }\
void set(t value) {\
m_##n = value;\
RaisePropertyChanged(L#n);\
Copy link
Contributor

@rudyhuyn rudyhuyn Jul 12, 2019

Choose a reason for hiding this comment

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

What are we trying to fix with it this macro? I don't think it's a good practice to use PropertyChanged to launch events when nothing changed, better to create a specific event ("Refresh"/"UserInteracted") for example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added this in because it handles a lot of work that I would need to do manually otherwise when binding a TextBox Text property to this property. For example, if a user types invalid input, this will let me set the Value to the previous Value and still fire the event, causing the binding to update, resetting the double in the textbox to the previous value with correct formatting.

I could just manually set the TextBox text in SubmitTextbox instead, but this would require extra work to also format the double -> string conversion to remove trailing zeroes. I noticed you added a utility in one of your recent PRs that lets us remove trailing zeroes very easily from a string. I added a comment to go ahead and remove this work around and use your util function once we re-merge with master and pick up that change.

{
auto variableViewModel = static_cast<VariableViewModel^>(sender->DataContext);

if (sender->Name == "Value")
Copy link
Contributor

Choose a reason for hiding this comment

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

to be sure to not break this function if the XAML is modified:

if(sender == Value)
{
...
}`

The names of these TextBox should probably be changed to remove some confusions and conflicts, I don't expect this->Value to be a TextBox for example:
proposal: ValueTextBox, MinTextBox, MaxTextBox, StepTextBox

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

sanderl
sanderl previously approved these changes Jul 23, 2019
@joseartrivera joseartrivera merged commit 46f11c7 into microsoft:feature/GraphingCalculator Jul 24, 2019
@joseartrivera joseartrivera added the graphing calculator Work items related to the graphing calculator feature. label Nov 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement graphing calculator Work items related to the graphing calculator feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants