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

Use amount input on rules page instead of plain text field #2566

Merged
merged 15 commits into from
Jun 5, 2024

Conversation

jfdoming
Copy link
Contributor

@jfdoming jfdoming commented Apr 8, 2024

There was feedback on the original rules-with-splits PR that amount inputs could use the featureful amount input instead of a plain text field. This PR updates that page to use the AmountInput component.

@trafico-bot trafico-bot bot added the 🔍 Ready for Review Pull Request is not reviewed yet label Apr 8, 2024
@github-actions github-actions bot changed the title Use amount input on rules page instead of plain text field [WIP] Use amount input on rules page instead of plain text field Apr 8, 2024
@trafico-bot trafico-bot bot added 🚧 WIP Still work-in-progress, please don't review and don't merge and removed 🔍 Ready for Review Pull Request is not reviewed yet labels Apr 8, 2024
Copy link

netlify bot commented Apr 8, 2024

Deploy Preview for actualbudget ready!

Name Link
🔨 Latest commit e838b27
🔍 Latest deploy log https://app.netlify.com/sites/actualbudget/deploys/665f0ff5bdf8360008dbec92
😎 Deploy Preview https://deploy-preview-2566.demo.actualbudget.org
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Contributor

github-actions bot commented Apr 8, 2024

Bundle Stats — desktop-client

Hey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle.

As this PR is updated, I'll keep you updated on how the bundle size is impacted.

Total

Files count Total bundle size % Changed
9 4.72 MB → 4.72 MB (+3.29 kB) +0.07%
Changeset
File Δ Size
src/components/util/PercentInput.tsx 🆕 +2.17 kB 0 B → 2.17 kB
src/components/util/GenericInput.jsx 📈 +918 B (+17.47%) 5.13 kB → 6.03 kB
src/components/spreadsheet/useFormat.ts 📈 +49 B (+3.91%) 1.22 kB → 1.27 kB
src/components/modals/EditRule.jsx 📈 +176 B (+0.50%) 34.27 kB → 34.45 kB
View detailed bundle breakdown

Added

No assets were added

Removed

No assets were removed

Bigger

Asset File Size % Changed
static/js/index.js 3 MB → 3.01 MB (+3.29 kB) +0.11%

Smaller

No assets were smaller

Unchanged

Asset File Size % Changed
static/js/resize-observer.js 18.37 kB 0%
static/js/indexeddb-main-thread-worker-e59fee74.js 13.5 kB 0%
static/js/BackgroundImage.js 122.29 kB 0%
static/js/usePreviewTransactions.js 790 B 0%
static/js/AppliedFilters.js 20.41 kB 0%
static/js/narrow.js 59.97 kB 0%
static/js/wide.js 263.11 kB 0%
static/js/ReportRouter.js 1.23 MB 0%

Copy link
Contributor

github-actions bot commented Apr 8, 2024

Bundle Stats — loot-core

Hey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle.

As this PR is updated, I'll keep you updated on how the bundle size is impacted.

Total

Files count Total bundle size % Changed
1 1.2 MB 0%

Changeset

No files were changed

View detailed bundle breakdown

Added

No assets were added

Removed

No assets were removed

Bigger

No assets were bigger

Smaller

No assets were smaller

Unchanged

Asset File Size % Changed
kcab.worker.js 1.2 MB 0%

@jfdoming jfdoming changed the title [WIP] Use amount input on rules page instead of plain text field Use amount input on rules page instead of plain text field Apr 8, 2024
@trafico-bot trafico-bot bot added 🔍 Ready for Review Pull Request is not reviewed yet and removed 🚧 WIP Still work-in-progress, please don't review and don't merge labels Apr 8, 2024
@ByteChild
Copy link

Hi @jfdoming

Unfortunately, this is not yet working as expected.
If I enter the value separated by a dot, it is automatically corrected to a comma.

However, if I enter the value separated by a comma, the value is set to 0,00 after saving.

I have created a video of the procedure to make it easier to understand:

https://youtu.be/Uq6b6-c8Zao

@joel-jeremy
Copy link
Contributor

Tried it out and looks like the sign always turns into positive when updating a negative amount atm

@trafico-bot trafico-bot bot added ⚠️ Changes requested Pull Request needs changes before it can be reviewed again and removed 🔍 Ready for Review Pull Request is not reviewed yet labels Apr 9, 2024
@ByteChild
Copy link

Tried it out and looks like the sign always turns into positive when updating a negative amount atm

Hi @joel-jeremy,

It's not about the signs.
It's about setting up a split as a rule. For example, I take a total of €10 and divide it into 2 splits of €5 each.

However, as soon as I enter the value separated by a comma (German notation), the value is not saved, but 0,00 instead.

@trafico-bot trafico-bot bot added 🔍 Ready for Review Pull Request is not reviewed yet and removed ⚠️ Changes requested Pull Request needs changes before it can be reviewed again labels Apr 9, 2024
@jfdoming
Copy link
Contributor Author

jfdoming commented Apr 9, 2024

hey @ByteChild @joel-jeremy, I believe the fix to just use onUpdate fixes both of your issues. Can you check when you have a moment?

@ByteChild
Copy link

It looks fine now :) Thank you @jfdoming and @joel-jeremy

@joel-jeremy
Copy link
Contributor

joel-jeremy commented Apr 9, 2024

Nice! That fixed the issue. Another one I see is the split rule's percent option is using the AmountInput component. I think that should stay as a regular input. Not sure how hard it would be to implement but maybe we can auto append % at the end of the input the user is typing (you don't have to do it in this PR)?

@jfdoming
Copy link
Contributor Author

Nice! That fixed the issue. Another one I see is the split rule's percent option is using the AmountInput component. I think that should stay as a regular input. Not sure how hard it would be to implement but maybe we can auto append % at the end of the input the user is typing (you don't have to do it in this PR)?

Good idea! I added a basic implementation of the % formatting that seemed to work well in my basic testing. Can you have a look when you get the time and let me know if you have any concerns with this? It behaves a little differently than the AmountInput component but IMO that's fine.

@joel-jeremy
Copy link
Contributor

When I set a fixed amount value e.g. 200 and then change to a percent amount, the default percent amount gets set to a large number (20000%)

@jfdoming
Copy link
Contributor Author

jfdoming commented Apr 14, 2024

When I set a fixed amount value e.g. 200 and then change to a percent amount, the default percent amount gets set to a large number (20000%)

Ah, good catch. That's a latent bug that was hidden because everything was using AmountInput. Fixed!

Edit: actually, still need to do some bugfixing around existing data. Should be fixed now!

Copy link
Contributor

@joel-jeremy joel-jeremy left a comment

Choose a reason for hiding this comment

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

There is a bug where percent results in a negative number when you switch from fixed amount and percent while the fixed amount input has a negative value in it.

@jfdoming
Copy link
Contributor Author

There is a bug where percent results in a negative number when you switch from fixed amount and percent while the fixed amount input has a negative value in it.

Hmm, yeah I was aware of that one. I think it will be tricky to get right due to the way state flows, but I'll give it another crack

@joel-jeremy
Copy link
Contributor

Nice! The negative issue seems to be fixed now. I wonder if it makes sense to always just default the percent to 100% instead of converting the amount to a percent i.e. 20000 --> 20000%

@trafico-bot trafico-bot bot added 🔍 Ready for Review Pull Request is not reviewed yet and removed ⚠️ Changes requested Pull Request needs changes before it can be reviewed again labels Jun 4, 2024
@jfdoming jfdoming force-pushed the jfdoming/amount-input-in-rules branch from 891eb94 to e838b27 Compare June 4, 2024 13:00
@jfdoming jfdoming requested a review from psybers June 4, 2024 13:02
@joel-jeremy
Copy link
Contributor

Functionally it's now working good! One nit I have UX wise is the movement of the cursor when you click on the input and also when you type non-numeric values. Makes it look like the input is flickering. Would it be better if we render a fixed % sign at the end of the percent input field instead of having the % as part of the input itself?

@psybers
Copy link
Contributor

psybers commented Jun 4, 2024

I agree the UX could be improved a bit (I see a couple of things), but I think this is pretty good for a first go. I'd recommend merging this and then focus on various UX improvements in future PRs.

@trafico-bot trafico-bot bot added ✅ Approved Pull Request has been approved and can be merged and removed 🔍 Ready for Review Pull Request is not reviewed yet labels Jun 4, 2024
@youngcw youngcw merged commit b89a320 into actualbudget:master Jun 5, 2024
19 checks passed
@trafico-bot trafico-bot bot added ✨ Merged Pull Request has been merged successfully and removed ✅ Approved Pull Request has been approved and can be merged ✨ Merged Pull Request has been merged successfully labels Jun 5, 2024
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.

None yet

5 participants