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

feat(importAccount): typeahead support using searchindex #2422

Merged
merged 1 commit into from
Mar 30, 2022

Conversation

aewing
Copy link
Contributor

@aewing aewing commented Mar 19, 2022

What this PR does πŸ“–
Implements a typeahead filter on the import account page

Which issue(s) this PR fixes πŸ”¨
AP-322

Special notes for reviewers πŸ—’οΈ
Made some changes to the active item logic:

  • Only one item can be active, mouse and keyboard will now take focus from one another
  • Pressing enter immediately after typing will now select the first match in the list

Additional comments 🎀
extends #2179

@github-actions github-actions bot added the Missing Dev Review A Dev and a Dev Approver need to review the PR, then mark as Ready for QA label Mar 19, 2022
@stavares843 stavares843 added depending on other PR Blocked by other PR. Once the other PR has gone in, rebase this branch to dev and test. draft A developer wants eyes on this PR, but they don't think it's ready to merge. and removed Missing Dev Review A Dev and a Dev Approver need to review the PR, then mark as Ready for QA labels Mar 19, 2022
@netlify
Copy link

netlify bot commented Mar 19, 2022

βœ… Yeeeehaw, deploy preview is ready!

Name Link
πŸ”¨ Latest commit 96ccc75
πŸ” Latest deploy log https://app.netlify.com/sites/adoring-edison-dbcef8/deploys/62435e3796ef4e000825764b
😎 Deploy Preview https://deploy-preview-2422--adoring-edison-dbcef8.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.

@stavares843
Copy link
Member

/rebase

@github-actions github-actions bot added the missing fixing conflict Conflict needs to be handled, then re-tested by devs/qa label Mar 21, 2022
@stavares843 stavares843 removed the depending on other PR Blocked by other PR. Once the other PR has gone in, rebase this branch to dev and test. label Mar 23, 2022
@stavares843
Copy link
Member

@aewing the other PR was merged, is this one still draft?

@aewing aewing marked this pull request as ready for review March 23, 2022 19:17
@aewing aewing added Ready for QA Ready for QA team to test, Devs approved. Missing Dev Review A Dev and a Dev Approver need to review the PR, then mark as Ready for QA and removed missing fixing conflict Conflict needs to be handled, then re-tested by devs/qa draft A developer wants eyes on this PR, but they don't think it's ready to merge. labels Mar 23, 2022
@phillsatellite
Copy link
Contributor

phillsatellite commented Mar 23, 2022

Tested: functionality works great but the list seems to be endless now and user cannot scroll to the bottom of it

Screen Shot 2022-03-23 at 5 16 28 PM

import.3.mov

@phillsatellite phillsatellite added Changes Requested Changes have been requested by Dev or QA, once the changes have been made, remove this label. and removed Ready for QA Ready for QA team to test, Devs approved. labels Mar 23, 2022
Copy link
Member

@stavares843 stavares843 left a comment

Choose a reason for hiding this comment

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

Gravacao.do.ecra.2022-03-24.as.13.50.54.mov

this branch vs dev

import account is broken on this branch, video above

if you copy paste a whole key, only pastes the first word

@aewing
Copy link
Contributor Author

aewing commented Mar 25, 2022

Thanks for finding these. Addressing these issues this morning, will have some updates soon.

@aewing aewing added Ready for QA Ready for QA team to test, Devs approved. and removed Changes Requested Changes have been requested by Dev or QA, once the changes have been made, remove this label. labels Mar 25, 2022
@stavares843
Copy link
Member

this is reverting this modification we did - #1499

basically, the first keyword shouldn't be highlighted

this branch

Captura de ecrã 2022-03-25, às 19 52 10

dev

Captura de ecrã 2022-03-25, às 19 52 18

@stavares843 stavares843 added Changes Requested Changes have been requested by Dev or QA, once the changes have been made, remove this label. and removed Ready for QA Ready for QA team to test, Devs approved. labels Mar 25, 2022
@aewing
Copy link
Contributor Author

aewing commented Mar 28, 2022

this is reverting this modification we did - #1499

basically, the first keyword shouldn't be highlighted

I can revert this change, but I found this to be flow breaking personally. I might save a few keypresses by having the item selected automatically and being able to press enter to continue to the next word (which is what I am accustomed to as a user with typeahead inputs). Reviewing the linked ticket, there isn't much detail about the 'why' behind this, so I thought perhaps it was related to a bug with the dual states of mouse hover and keyboard which have been resolved.

@aewing
Copy link
Contributor Author

aewing commented Mar 28, 2022

@phillsatellite can you provider clarity on what we were actually solving for with #1499?

@stavares843
Copy link
Member

basically, the first word on that PR was always highlighted, and Alex fix that

if you go to dev, the first word is not highlighted

but on this PR, was being highlighted again

basically we don't want words highlights when user is still searching unless user is hovering on those

@stavares843 stavares843 added temporary blocked checking something QA Lead is checking something. and removed Changes Requested Changes have been requested by Dev or QA, once the changes have been made, remove this label. labels Mar 28, 2022
@stavares843
Copy link
Member

for speed's sake, not against in not reverting the above

cc @WanderingHogan

@aewing
Copy link
Contributor Author

aewing commented Mar 29, 2022

basically, the first word on that PR was always highlighted, and Alex fix that

if you go to dev, the first word is not highlighted

but on this PR, was being highlighted again

basically we don't want words highlights when user is still searching unless user is hovering on those

Yea, I see what you're saying. I'm just trying to understand why we would not want this behavior, as it is the default behavior for as-you-type/typeahead components everywhere. I will revert the change today, but thought it worth having the discussion because the default selection feels right to me as a user.

@WanderingHogan
Copy link
Contributor

I think this is good to go in if the functionality is there. The typeahead got very complex and it didn't need to be. We shouldn't add overrides for everything and try to steer towards using things without a million overrides

@WanderingHogan WanderingHogan removed the Missing Dev Review A Dev and a Dev Approver need to review the PR, then mark as Ready for QA label Mar 30, 2022
@stavares843
Copy link
Member

sounds good πŸ”¨

@WanderingHogan
Copy link
Contributor

πŸš€ πŸŒ”

@stavares843 stavares843 merged commit 462c254 into dev Mar 30, 2022
@stavares843 stavares843 deleted the AP-322-typeahead branch March 30, 2022 23:46
@github-actions github-actions bot removed the temporary blocked checking something QA Lead is checking something. label Mar 30, 2022
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

4 participants