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

Render on visible for better performance #101

Merged
merged 3 commits into from Dec 3, 2017

Conversation

Projects
None yet
3 participants
@t9md
Contributor

t9md commented Nov 23, 2017

Description of the Change

[Updated]

Aiming to solve #80
Depends on atom/atom-select-list#22

This PR use new atom select-list's initiallyVisibleItemCount(not merged yet) option.
When this option was passed, new visible option passed to elementForItem reflect visibility state in viewport.

We can use this information to skip heavy computation for faster response.
Here I just return empty li element when invisible.

Here is behavioral changes in command-palette.

  • Old:
    • All items are equally rendered with command name, keymap shortcut.
  • New:
    • First 10 items are always rendered with full info, others are just rendered with empty li.
    • When item become visible, render with full information(command name, shortcut)

Alternate Designs

#81
#98
#97

Benefits

Performance improve.
command-palette launch faster.

Perf comparison is at atom/atom-select-list#22.

Plus, user can easily notice perf improvement especially for initial launch, and user have many commands(like installing vim-mode-plus or nuculide).

Possible Drawbacks

When mouse scroll very quickly, user can see blank fake item.

Applicable Issues

#80

@leroix

This comment has been minimized.

Show comment
Hide comment
@leroix

leroix Nov 26, 2017

Contributor

@t9md I checked out your branch and linked your atom-select-list branch. It works well, and the timeline tab definitely shows it to be faster in loading the command palette (<100ms vs 250ms). Nice work!

I think the behaviors you mentioned would be more appropriately tested in atom/atom-select-list#22.

Contributor

leroix commented Nov 26, 2017

@t9md I checked out your branch and linked your atom-select-list branch. It works well, and the timeline tab definitely shows it to be faster in loading the command palette (<100ms vs 250ms). Nice work!

I think the behaviors you mentioned would be more appropriately tested in atom/atom-select-list#22.

@t9md

This comment has been minimized.

Show comment
Hide comment
@t9md

t9md Nov 26, 2017

Contributor

@leroix Thanks you for feedback.
I worked on different issue in a few days.
But I'll continue to add some spec at select-list repo(but I still have difficulty how to do it)

Contributor

t9md commented Nov 26, 2017

@leroix Thanks you for feedback.
I worked on different issue in a few days.
But I'll continue to add some spec at select-list repo(but I still have difficulty how to do it)

@t9md t9md referenced this pull request Nov 27, 2017

Closed

Dyamic max results #98

@t9md t9md closed this Dec 1, 2017

@t9md t9md reopened this Dec 1, 2017

@leroix

This comment has been minimized.

Show comment
Hide comment
@leroix

leroix Dec 3, 2017

Contributor

@t9md is this PR ready to be merged?

Contributor

leroix commented Dec 3, 2017

@t9md is this PR ready to be merged?

@t9md

This comment has been minimized.

Show comment
Hide comment
@t9md

t9md Dec 3, 2017

Contributor

Yes

Contributor

t9md commented Dec 3, 2017

Yes

@leroix leroix merged commit acc4cef into atom:master Dec 3, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Dec 10, 2017

Member

@t9md I was wondering if you think this will be useful to implement in all other Atom core packages that use atom-select-list?

Member

50Wliu commented Dec 10, 2017

@t9md I was wondering if you think this will be useful to implement in all other Atom core packages that use atom-select-list?

@t9md

This comment has been minimized.

Show comment
Hide comment
@t9md

t9md Dec 11, 2017

Contributor

Not sure if it's always good to introduce this new render-on-visible feature.
Theoretically, it improve perf by reducing amount of initial rendering item.
But if perf is not currently issue in that package, keeping it as-is is OK.
One minor caveat of this render-on-visible(enabled by passing initiallyVisibleItemCount) feature is "when user really scroll fast, user can see blank item on the list" since IntersectionObserver works in async, having delay to replace to real item element after it become visible.

Contributor

t9md commented Dec 11, 2017

Not sure if it's always good to introduce this new render-on-visible feature.
Theoretically, it improve perf by reducing amount of initial rendering item.
But if perf is not currently issue in that package, keeping it as-is is OK.
One minor caveat of this render-on-visible(enabled by passing initiallyVisibleItemCount) feature is "when user really scroll fast, user can see blank item on the list" since IntersectionObserver works in async, having delay to replace to real item element after it become visible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment