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

fix(select): default value not being centered when not visible #59

Merged
merged 7 commits into from
Sep 8, 2023

Conversation

toyi
Copy link
Contributor

@toyi toyi commented Sep 8, 2023

When you have a select prompt and set a default value outside the first x values (where x is defined by the scroll parameter), you don't see it.

It's even worst when you scroll down the list, since you'll never see what is currently selected, it'll remain outside of the displayed values until the end, when it loops back to the top.

This PR fixes this by centering the "view" (the displayed values) on the default item.

I had to modify the select.php playground in order to reflect this change.

Before:

Recording.2023-09-08.093337.mp4

After:

Recording.2023-09-08.093456.mp4

...and when the default value is near the bottom of the list (in this example with a scroll 10):

Recording.2023-09-08.131053.mp4

src/SelectPrompt.php Outdated Show resolved Hide resolved
@jessarcher
Copy link
Member

Good catch! Thanks for fixing it!

Unfortunately, the issue still exists when the terminal height can't accommodate the configured scroll value.

It's not straightforward to solve, though. Currently, the renderer reduces the prompt's scroll property if needed, considering any additional lines it needs to render above and below the scrolling section. This happens after the prompt is constructed, which is too late.

I'm working on a solution so that renderers can expose the number of lines they need to reserve, and then the prompt itself can adjust the scroll property.

@jessarcher jessarcher marked this pull request as draft September 8, 2023 10:50
src/Concerns/Fallback.php Outdated Show resolved Hide resolved
@toyi
Copy link
Contributor Author

toyi commented Sep 8, 2023

Good catch! Thanks for fixing it!

Unfortunately, the issue still exists when the terminal height can't accommodate the configured scroll value.

It's not straightforward to solve, though. Currently, the renderer reduces the prompt's scroll property if needed, considering any additional lines it needs to render above and below the scrolling section. This happens after the prompt is constructed, which is too late.

I'm working on a solution so that renderers can expose the number of lines they need to reserve, and then the prompt itself can adjust the scroll property.

Good catch too, I didn't even think about it!

@jessarcher
Copy link
Member

I'm working on a solution so that renderers can expose the number of lines they need to reserve, and then the prompt itself can adjust the scroll property.

I've submitted #60, which makes sure the $scroll property is correctly set before your new scroll logic.

@toyi
Copy link
Contributor Author

toyi commented Sep 8, 2023

I've submitted #60, which makes sure the $scroll property is correctly set before your new scroll logic.

Thank you, I've updated my main branch and deleted the now useless disableFallback method.

@jessarcher
Copy link
Member

Thanks!

@jessarcher jessarcher marked this pull request as ready for review September 8, 2023 12:59
@jessarcher
Copy link
Member

Somewhat related to this PR (and #58), I've just been playing around with showing how many items have been selected in a multi-select, which is especially helpful when they're not all on-screen.

image

@taylorotwell taylorotwell merged commit c59734c into laravel:main Sep 8, 2023
3 checks passed
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.

3 participants