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 buttons for organize column search #1620

Merged

Conversation

ethanalvizo
Copy link
Contributor

Closes #1529

@ethanalvizo ethanalvizo self-assigned this Nov 3, 2023
@@ -1049,6 +1109,14 @@ class VisibilityOrderingBuilder extends Component<
treeItems
);

const queryParams = {
Copy link
Contributor Author

@ethanalvizo ethanalvizo Nov 3, 2023

Choose a reason for hiding this comment

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

i thought this would be nicer to pass in to SearchInput rather than creating two new optional props that would only be used by VisibilityOrderingBuilder. Could revert to that if preferred though

* @param direction The direction to move the selection
*/

changeSelectedColumn(direction: 'forward' | 'back'): 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 feel this could use a better name but not too sure what. This does change the selected column as the name implies but only from the previously queried list

@ethanalvizo ethanalvizo enabled auto-merge (squash) November 3, 2023 04:42
Copy link

codecov bot commented Nov 3, 2023

Codecov Report

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

Comparison is base (52ba2cd) 46.63% compared to head (4712199) 46.62%.
Report is 5 commits behind head on main.

❗ Current head 4712199 differs from pull request most recent head 1d43453. Consider uploading reports for the commit 1d43453 to get more accurate results

Files Patch % Lines
packages/components/src/SearchInput.tsx 0.00% 31 Missing ⚠️
...ity-ordering-builder/VisibilityOrderingBuilder.tsx 72.58% 17 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1620      +/-   ##
==========================================
- Coverage   46.63%   46.62%   -0.02%     
==========================================
  Files         591      592       +1     
  Lines       36406    36516     +110     
  Branches     9113     9154      +41     
==========================================
+ Hits        16979    17026      +47     
- Misses      19375    19438      +63     
  Partials       52       52              
Flag Coverage Δ
unit 46.62% <48.38%> (-0.02%) ⬇️

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.

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.

The column search going back to the selected item, instead of picking up from your current selection seems counter intuitive to most find interfaces. To wit, if I search for “log” in intellij; hit the button a few times to cycle; then select something else with my mouse and hit the button again it picks up the search from my cursor not the “n / m” I was on.

Arrows should go to the previous/next one before/after a manually selected index:

ex.
Search A, click BB, hit next get AC.
AA
AB
BB
AC
AD

@dsmmcken
Copy link
Contributor

Scroll into view behaviour doesn't appear to be functioning correctly.

scroll_into_view

@dsmmcken
Copy link
Contributor

My guess is a react re-render is interrupting the scroll.

@ethanalvizo
Copy link
Contributor Author

My guess is a react re-render is interrupting the scroll.

fixed in the latest version now. It wasn't a re-render. My newest changes just broke the previous way I was passing in which item to focus on. All good now though!

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.

Approved for functionality, didn't code review.

@ethanalvizo ethanalvizo merged commit 75cf184 into deephaven:main Nov 14, 2023
4 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Nov 14, 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
2 participants