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

♻️ (autocomplete) refactor PayeeAutocomplete to react-select #741

Merged
merged 22 commits into from
Mar 17, 2023

Conversation

MatissJanis
Copy link
Member

@MatissJanis MatissJanis commented Mar 11, 2023

Refactored PayeeAutocomplete to react-select. The new component is under a feature flag.

I will be slowly refactoring other bits too, but it will take time as it's not an easy migration.. so I've put it under a feature flag for now. Once we're ready to release and are confident we can remove the old code.

This will also allow us to eventually remove lively.

There might be some small functional/design differences. I tried getting it as close to the existing one as possible, but it won't be 1:1.

@netlify
Copy link

netlify bot commented Mar 11, 2023

Deploy Preview for actualbudget ready!

Name Link
🔨 Latest commit 462c0d2
🔍 Latest deploy log https://app.netlify.com/sites/actualbudget/deploys/6414da54afcb5200086396ec
😎 Deploy Preview https://deploy-preview-741--actualbudget.netlify.app/
📱 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 settings.

@MatissJanis MatissJanis marked this pull request as ready for review March 11, 2023 18:38
@SalvyTek
Copy link

I did a few clicks with the new version of autocomplete, and I found this little bug.

Screenshot (4)

@j-f1
Copy link
Contributor

j-f1 commented Mar 11, 2023

Very small nit: the highlighted row indicators should be clipped by the rounded corners. Instead they poke out:

(EDIT: oops this is with the account one not the payee one but I see the same issue there)

@MatissJanis
Copy link
Member Author

Fix highlighted rows overlaying autocomplete

Good spot! I really dislike these zIndex shenanigans, but.. at least now the issue is solved: 2369fdf

Very small nit: the highlighted row indicators should be clipped by the rounded corners. Instead they poke out:

(EDIT: oops this is with the account one not the payee one but I see the same issue there)

Yea, there is a small bottom padding. Not worth fixing IMO

Screenshot 2023-03-11 at 21 27 37

Copy link
Contributor

@j-f1 j-f1 left a comment

Choose a reason for hiding this comment

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

A few more issues:

  • Moving your cursor out of the menu will scroll it to the top, it would be nice to preserve scroll position.
  • Tab/Shift+Tab do not properly move to the previous and next columns
  • esc should close the menu while keeping focus on the text field (so that nearby rows can be reviewed while entering a value)
  • The old autocomplete UI doesn’t reliably do this but it would be awesome to consistently flip the dropdown to appear above the field if there isn’t enough space below it.
  • Would it be possible to reduce the top padding here so that the title nestles more snugly into the corner?
    Screenshot_2023-03-11 17 07 25

@trafico-bot trafico-bot bot added ⚠️ Changes requested Pull Request needs changes before it can be reviewed again and removed 🔍 Ready for Review labels Mar 11, 2023
@j-f1
Copy link
Contributor

j-f1 commented Mar 11, 2023

Yea, there is a small bottom padding. Not worth fixing IMO

For a component as common and essential as this I think we should fix all the small issues we can. Happy to work on it myself though!

@trafico-bot trafico-bot bot added 🔍 Ready for Review and removed ⚠️ Changes requested Pull Request needs changes before it can be reviewed again labels Mar 11, 2023
@MatissJanis
Copy link
Member Author

Added a few small style tweaks. Will see if I can fix some of the functional issues too in the coming days. Signing off for today :)

Feel free to also work on this branch and push your changes if you have some time.

@MatissJanis
Copy link
Member Author

MatissJanis commented Mar 12, 2023

Solved some more problems:

  • Moving your cursor out of the menu will scroll it to the top, it would be nice to preserve scroll position.
  • Tab/Shift+Tab do not properly move to the previous and next columns
  • esc should close the menu while keeping focus on the text field (so that nearby rows can be reviewed while entering a value)
  • The old autocomplete UI doesn’t reliably do this but it would be awesome to consistently flip the dropdown to appear above the field if there isn’t enough space below it.
  • Would it be possible to reduce the top padding here so that the title nestles more snugly into the corner?

@MatissJanis
Copy link
Member Author

Ok, I'm stuck on the "tab" issue and the scrolling issue ((1) and (2) in the list). A fresh perspective on the issue (another pair of eyes) would be very much appreciated.

@MatissJanis
Copy link
Member Author

Tab key fixed

@j-f1
Copy link
Contributor

j-f1 commented Mar 14, 2023

Looking at the scrolling issue now

@j-f1
Copy link
Contributor

j-f1 commented Mar 14, 2023

I figured it out! The problem is that React unmounts and remounts components when their identities change, and the value of the function defining the component is part of that identity. As a result, using a function expression as a component will cause it to unmount and remount each time the component defining the function is re-rendered. I’ve extracted the component out to fix this issue. (And also created a NullComponent for cases where that’s desired).

@MatissJanis
Copy link
Member Author

Amazing! Great job @j-f1 !

@j-f1
Copy link
Contributor

j-f1 commented Mar 14, 2023

I'm not sure if this is relevant, but a similar thing will happen if you switch between the Creatable and Select components.

@MatissJanis
Copy link
Member Author

I'm not sure if this is relevant, but a similar thing will happen if you switch between the Creatable and Select components.

That shouldn't be a problem. We generally don't switch between the two states. The component is always either one or the other (depends on the part of the app).

@MatissJanis
Copy link
Member Author

MatissJanis commented Mar 14, 2023

Noticed a bug in the rules modal: the selection box was pushing the content down which was very ugly. So moved the popover to a portal + fixed the subsequent issues.

Oh and also the top/bottom direction of the popover is now dynamic.

The only remaining issue I'm spotting now is: it's not possible to set a payee filter on the transaction table.

@MatissJanis
Copy link
Member Author

Fixed the filter list issue. Out for review again :)

@j-f1
Copy link
Contributor

j-f1 commented Mar 15, 2023

Fixed a minor flickering issue when scrolling:

content warning: flickering image
Screen.Recording.2023-03-14.at.20.06.18.mov

@j-f1
Copy link
Contributor

j-f1 commented Mar 15, 2023

  • When scrolling the focused row out of view it suddenly jumps back into view. The old component gracefully drops focus and picks it up when scrolled back into view
  • Pressing esc should reset the value of the select to its initial value like the old select does
  • fix the spacing of the “Create” menu item
  • The payee name cannot be selected (using text selection), unlike in the old one. This makes it hard to switch between very similar payee names
  • not sure the “x” button provides much value and it is super weird (it acts on mouse down + delay rather than mouse up), maybe it can be removed?
  • global undo/redo don’t work when focused on a text field
  • there’s a noticeable delay when clicking the “Make Transfer” button, it would be nice for that to be instant

Copy link
Contributor

@j-f1 j-f1 left a comment

Choose a reason for hiding this comment

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

I’m happy merging this for now so you don’t have to keep fixing merge conflicts and then we can iterate on the remaining problems before dropping the old code.

@MatissJanis
Copy link
Member Author

I’m happy merging this for now so you don’t have to keep fixing merge conflicts and then we can iterate on the remaining problems before dropping the old code.

Good idea! Thanks for creating the ticket!

@MatissJanis MatissJanis merged commit 141035c into master Mar 17, 2023
@MatissJanis MatissJanis deleted the matiss/autocomplete branch March 17, 2023 22:36
@trafico-bot trafico-bot bot added ✨ Merged Pull Request has been merged successfully and removed 🔍 Ready for Review labels Mar 17, 2023
FlorianLang06 pushed a commit to FlorianLang06/actual that referenced this pull request Mar 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✨ Merged Pull Request has been merged successfully
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants