Skip to content

Conversation

ManiacHanz
Copy link
Contributor

  • Change params of scrollTo from ScrollConfig to (number | ScrollConfig)
    issue-link

@vercel
Copy link

vercel bot commented Mar 18, 2022

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/react-component/select/A1bY93VcfpUTTqnBxyZFXShCeNq3
✅ Preview: https://select-git-fork-maniachanz-fix-scroll-params-react-component.vercel.app

onKeyDown: React.KeyboardEventHandler;
onKeyUp: React.KeyboardEventHandler;
scrollTo?: (index: number) => void;
scrollTo?: (args: number | ScrollConfig) => 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.

Since the virtual list hereused (arg: number | ScrollConfig) as its params type, so I think maybe we should allow here to accept type ScrollConfig

});
wrapper.update();
ref.current.scrollTo(100);
ref.current.scrollTo(scrollParams);
Copy link
Member

Choose a reason for hiding this comment

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

Please add a new test case for this.

@codecov
Copy link

codecov bot commented Mar 19, 2022

Codecov Report

Merging #728 (40b0a47) into master (65c96dd) will not change coverage.
The diff coverage is 100.00%.

❗ Current head 40b0a47 differs from pull request most recent head 76e14d0. Consider uploading reports for the commit 76e14d0 to get more accurate results

@@           Coverage Diff           @@
##           master     #728   +/-   ##
=======================================
  Coverage   99.50%   99.50%           
=======================================
  Files          25       25           
  Lines        1011     1011           
  Branches      320      320           
=======================================
  Hits         1006     1006           
  Misses          4        4           
  Partials        1        1           
Impacted Files Coverage Δ
src/BaseSelect.tsx 100.00% <100.00%> (ø)
src/OptionList.tsx 99.28% <100.00%> (ø)

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@MadCcc
Copy link
Member

MadCcc commented Mar 21, 2022

LGTM

@afc163 afc163 merged commit 60cdccd into react-component:master Mar 31, 2022
const scrollIntoView = (args: number | ScrollConfig) => {
if (listRef.current) {
listRef.current.scrollTo({ index });
listRef.current.scrollTo(args);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
listRef.current.scrollTo(args);
listRef.current.scrollTo(typeof args === 'number' ? { index: args } : args);

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.

4 participants