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

Financial Calculator #22

Open
wants to merge 50 commits into
base: finance
Choose a base branch
from
Open

Conversation

Chips1234
Copy link
Contributor

Financial Calculator

Feature Pitch Link: microsoft/calculator#806

@Chips1234
Copy link
Contributor Author

Chips1234 commented Jul 12, 2020

Also, can anyone help me with the booleantovisibility conversion? (see gif below, it doesn’t seem to hide the other grid when loading another, it also can’t change back)
Calculator Financial Mode Change

@Chips1234
Copy link
Contributor Author

@joseartrivera? @sanderl? What do you think?

@sanderl
Copy link

sanderl commented Jul 14, 2020

Hi @Chips1234, Thanks so much for writing the spec for Financial Calculator. @grochocki will be able to answer your questions about the spec. He is pretty busy so it may take a bit for him to get back to you.

@Chips1234
Copy link
Contributor Author

@EriWong could you take a look? And possibly the other pr's (they've been inactive for a while now) thanks!

@EriWong
Copy link

EriWong commented Jul 27, 2020

@Chips1234 Sorry for the delay. I'll see if I can bring this up with for our next set of feature pitch reviews. If there's anything we find that you need to add to this, we'll let you know.

As an aside, if you are working on a code sample and have questions about implementations, if you share with us your repo/branch information, we can probably more easily help you out in figuring out what's happening (with your conversion or with other things as well).

@Chips1234
Copy link
Contributor Author

Chips1234 commented Jul 27, 2020

@Chips1234 Sorry for the delay. I'll see if I can bring this up with for our next set of feature pitch reviews. If there's anything we find that you need to add to this, we'll let you know.

Thanks!

As an aside, if you are working on a code sample and have questions about implementations, if you share with us your repo/branch information, we can probably more easily help you out in figuring out what's happening (with your conversion or with other things as well).

You can look at my branch here. Thanks again!

@EriWong
Copy link

EriWong commented Jul 28, 2020

@Chips1234 I think I figured out the converter issue you encountered and it's a tricky one.

So I don't think m_FinanceCalcViewModel is actually getting set when navigating. As a result, I'm not exactly sure what the view is bound to... At any rate, I instatiated that, and am now hitting an error where IsCompound is not defined in your viewmodel. This now makes more sense as I think you're casing in the viewmodel has it as isCompound. After fixing that, everything seems to work.

@Chips1234
Copy link
Contributor Author

@Chips1234 I think I figured out the converter issue you encountered and it's a tricky one.

So I don't think m_FinanceCalcViewModel is actually getting set when navigating. As a result, I'm not exactly sure what the view is bound to... At any rate, I instatiated that, and am now hitting an error where IsCompound is not defined in your viewmodel. This now makes more sense as I think you're casing in the viewmodel has it as isCompound. After fixing that, everything seems to work.

Ah, okay. Thank you!

@Chips1234
Copy link
Contributor Author

@Chips1234 I think I figured out the converter issue you encountered and it's a tricky one.

So I don't think m_FinanceCalcViewModel is actually getting set when navigating. As a result, I'm not exactly sure what the view is bound to... At any rate, I instatiated that, and am now hitting an error where IsCompound is not defined in your viewmodel. This now makes more sense as I think you're casing in the viewmodel has it as isCompound. After fixing that, everything seems to work.

@EriWong Sorry to bother your weekend but I still do not quite understand. Could you maybe share with me the code that you have? Thank you.

@EriWong
Copy link

EriWong commented Aug 17, 2020

@Chips1234 Sorry if I wasn't super clear. Take a look at ApplicationViewModel::OnModeChanged() (around line 126 in ApplicationViewModel.cpp).

In there, there is an if block around NavCategory::IsCalculatorViewMode(m_mode). Looking at the code there, we get the following check:

bool NavCategory::IsCalculatorViewMode(ViewMode mode)
{
    // Historically, Calculator modes are Standard, Scientific, and Programmer.
    return !IsDateCalculatorViewMode(mode) && !IsGraphingCalculatorViewMode(mode) && IsModeInCategoryGroup(mode, CategoryGroupType::Calculator);
}

The IsModeInCategoryGroup call is returning true. This is because your calculator type is marked as Calculator (as opposed to converter or something else). As such, this thinks you want to bind a standard calculator viewmodel to your financial calculator. It looks like date calculation and graphing calculator modes also fall into this pattern, hence the somewhat awkward if statement above.

A short term fix would be to add financial calculator to the IsCalculatorViewMode check (that's what date and graphing modes did). Longer term, we may want to clean this up to something a little clearer, but that's probably outside the scope of this change.

@Chips1234
Copy link
Contributor Author

Chips1234 commented Aug 17, 2020 via email

@EriWong
Copy link

EriWong commented Aug 17, 2020

@Chips1234 Once you populate m_FinanceCalcViewModel correctly, switching between tip and interest modes should work. The issues you were seeing were due to m_FinanceCalcViewModel not being correctly initialized.

@Chips1234
Copy link
Contributor Author

Chips1234 commented Aug 17, 2020 via email

@Chips1234
Copy link
Contributor Author

Here's my try at layout reflows:
Finance mode setter
If anyone can help me with it that'd be great. Thanks

@Chips1234
Copy link
Contributor Author

@grochocki any more suggestions?

@EriWong
Copy link

EriWong commented Sep 1, 2020

@Chips1234 apologies for the delay. With respect to reflow, it's probably worth looking at how other pages handle reflow. If you take a look at Calculator.xaml and take a look at <VisualStateGroup x:Name="LayoutVisualStates" ...> You can see how we handle it there.

@Chips1234
Copy link
Contributor Author

Hello @EriWong, if I have three options in the ComboBox (for example, I add the "Simple Interest" option), what would be a good way to show/hide the grids? Thank you.

@mdtauk
Copy link

mdtauk commented Jan 11, 2021

NumberBoxes or TextBoxes?

@Chips1234
Copy link
Contributor Author

@mdtauk, I am using NumberBoxes.

@mdtauk
Copy link

mdtauk commented Jan 11, 2021

If space becomes tight, you could switch the Number of spin buttons between inline and compact

@Chips1234
Copy link
Contributor Author

@mdtauk, Thank you for your tips. I'll do that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants