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: forward and back button for organize column search #1641

Merged

Conversation

ethanalvizo
Copy link
Contributor

Closes #1529

@ethanalvizo ethanalvizo self-assigned this Nov 14, 2023
@ethanalvizo ethanalvizo requested a review from mofojed November 14, 2023 15:30
Copy link

codecov bot commented Nov 14, 2023

Codecov Report

Attention: 38 lines in your changes are missing coverage. Please review.

Comparison is base (a2ef056) 46.65% compared to head (a6ba0a3) 46.62%.
Report is 5 commits behind head on main.

Files Patch % Lines
packages/components/src/SearchInput.tsx 0.00% 27 Missing ⚠️
...ity-ordering-builder/VisibilityOrderingBuilder.tsx 85.33% 11 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1641      +/-   ##
==========================================
- Coverage   46.65%   46.62%   -0.04%     
==========================================
  Files         593      597       +4     
  Lines       36467    36616     +149     
  Branches     9135     9172      +37     
==========================================
+ Hits        17014    17072      +58     
- Misses      19401    19492      +91     
  Partials       52       52              
Flag Coverage Δ
unit 46.62% <62.74%> (-0.04%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

packages/components/src/SearchInput.tsx Outdated Show resolved Hide resolved
Comment on lines 8 to 11
interface QueryParams {
queriedColumnIndex: number | undefined;
changeQueriedColumnIndex: (direction: 'forward' | 'back') => void;
}
Copy link
Member

Choose a reason for hiding this comment

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

These names are no good - SearchInput is a generic component, it doesn't/shouldn't know anything about the context it is being used in (in this case, it shouldn't know that it's querying columns).
In terms of using the component, there's a few different scenarios:

  • With or without a matchCount (you may or may not know the total number of results)
  • With or without a "current index" and options to go back/next

I think you just need more generic names here. So instead of queryParams?: QueryParams, should be:

cursor?: {
  index: number;  // No reason for `index` to be optional
  next: (direction: 'forward' | 'backward') => void;
}

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 commented on this below

@ethanalvizo ethanalvizo requested a review from mofojed November 15, 2023 22:31
Copy link
Contributor

@dsmmcken dsmmcken left a comment

Choose a reason for hiding this comment

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

Would you murder me if I ask for one two more things?

  1. When in the find field and there are results, enter and shift+enter should act as keyboard shortcuts for next/previous match, and should be exposed as the shortcut on the tooltip as well.

    Same shortcut as vs code:
    image

  2. Also the left button should say "Previous match (Shift+Enter)" and the right "Next match (Enter)". They are currently say the same thing.
    image

@ethanalvizo ethanalvizo requested a review from dsmmcken November 17, 2023 04:39
queriedColumnIndex,
changeQueriedColumnIndex: (direction: 'forward' | 'back') =>
const cursor = {
index: queriedColumnIndex as number,
Copy link
Member

Choose a reason for hiding this comment

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

You shouldn't need this cast, and in fact it's pointing out an error - you could be passing index: undefined here, which is not what SearchInput is expecting.
Instead, you should just check if it is not set and just return undefined for that instead of the parameter, e.g.:

    const cursor =
      queriedColumnIndex != null
        ? {
            index: queriedColumnIndex,
            next: (direction: 'forward' | 'back') =>
              this.changeSelectedColumn(direction),
          }
        : undefined;

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 unresolved one of your previous comments. I didn't think it through properly but queriedColumnIndex should be able to be undefined. When the user searches initially there is no index yet. But cursor should still be passed in since we need the next function for the forward and back buttons

Copy link
Contributor

@dsmmcken dsmmcken left a comment

Choose a reason for hiding this comment

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

Searching then doing previous/next starts at the end instead of the start.

  1. search a column
  2. hit next

expected:
Item selected is 1 of N

actual:
Item selected is Nth of N (which would be expected if I pressed previous)

Copy link
Contributor

@dsmmcken dsmmcken left a comment

Choose a reason for hiding this comment

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

Holding down the Enter key after searching will accumulate changes and then lock the UI while it applies all of them at once, rather than on key repeat.

@mofojed for advice.
Maybe because contextactions? Could use key events directly if that gives better control.

@ethanalvizo ethanalvizo requested a review from mofojed November 17, 2023 16:19
@dsmmcken
Copy link
Contributor

Holding down the Enter key after searching will accumulate changes and then lock the UI while it applies all of them at once, rather than on key repeat.

@mofojed for advice. Maybe because contextactions? Could use key events directly if that gives better control.

This is probably the issue #1650, poor drag performance. Can be ignored for this ticket, no changes needed here.

Copy link
Contributor

@dsmmcken dsmmcken left a comment

Choose a reason for hiding this comment

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

dimisses

Find gets dismissed if you search, click an item, click back in search, click back on an item.

Copy link
Contributor

@dsmmcken dsmmcken left a comment

Choose a reason for hiding this comment

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

scroll_into_view

Scroll into view isn't appropriately taking into account groups.

Have a table with a lot of columns, create two groups, one at the top with a bunch of columns, and a second group a few pages off screen. Search for the group name and attempt to have it scrolled into view. I think it ends up higher by the size of the first group.

@ethanalvizo ethanalvizo requested a review from dsmmcken November 23, 2023 06:52
@dsmmcken
Copy link
Contributor

image_360

I somehow managed to get this once, but completely unable to reproduce.

All my other issues are resolved.

Approved for functionality.

Copy link
Member

Choose a reason for hiding this comment

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

I'm a little confused how the delete icon is appearing in this search field - I don't see it when I'm testing on Linux. Seems odd but it was also there before your changes, so not going to block this change to figure out what's going on there.

Copy link
Contributor

Choose a reason for hiding this comment

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

type="search" on the input controls that I think.

@ethanalvizo ethanalvizo enabled auto-merge (squash) November 27, 2023 14:55
@ethanalvizo ethanalvizo merged commit 89f2be5 into deephaven:main Nov 27, 2023
5 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Nov 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add forward and back buttons to organize column search
3 participants