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

Add view state for scroll prompts #43

Merged
merged 8 commits into from
Aug 18, 2023

Conversation

crazywhalecc
Copy link
Contributor

@crazywhalecc crazywhalecc commented Aug 7, 2023

I tried to resolve the issue #42 I created earlier, and this PR completed this feature: you can naturally scroll according to the options displayed on the current page, with minimal cost (perhaps).

Untitled

As you know, I added a class to store view state, and changed the scroll behavior.

But I only make the virtical scroll naturally. The implementation of the horizontal direction is more complex, and the current plan in the horizontal direction feels tolerable.

@crazywhalecc crazywhalecc marked this pull request as draft August 7, 2023 14:33
@crazywhalecc crazywhalecc marked this pull request as ready for review August 7, 2023 15:04
@jessarcher
Copy link
Member

I'll need to do a more thorough test and code review, but my first impressions of the UX after checking this out locally are very positive! Well done! 🙌

@jessarcher jessarcher self-assigned this Aug 7, 2023
@taylorotwell taylorotwell marked this pull request as draft August 8, 2023 14:11
@taylorotwell
Copy link
Member

@jessarcher mark as ready please when you're ready for me to take a look 👍

@jessarcher
Copy link
Member

jessarcher commented Aug 18, 2023

Hey @crazywhalecc!

Sorry for the delay. Your scroll behaviour is perfect, but I've refactored the implementation to better fit my mental model. It also removes any state manipulation from the renderers so they can focus purely on rendering.

Thanks for your PR and for getting this feature happening!

@jessarcher jessarcher marked this pull request as ready for review August 18, 2023 07:49
@taylorotwell taylorotwell merged commit b514c56 into laravel:main Aug 18, 2023
3 checks passed
@crazywhalecc crazywhalecc deleted the view-state branch August 21, 2023 01:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants