-
Notifications
You must be signed in to change notification settings - Fork 30
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
Update preventScroll option in focus and focusElement #3177
base: develop
Are you sure you want to change the base?
Update preventScroll option in focus and focusElement #3177
Conversation
Enact-DCO-1.0-Signed-off-by: Nakjun Hwang <[email protected]>
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## develop #3177 /- ##
========================================
Coverage 82.21% 82.21%
========================================
Files 155 155
Lines 7141 7142 1
Branches 1885 1887 2
========================================
Hits 5871 5872 1
Misses 997 997
Partials 273 273
☔ View full report in Codecov by Sentry. |
@nakjun12 Thanks for the PR. Do you have any user scenarios you mentioned about "the forced scrolling behavior caused discomfort in the user experience"? It would be appreciated if you also explain the scenario how we can use this |
@seunghoh Thank you for your review. I encountered the issue while implementing an app with a user interface similar to Netflix. During development, I noticed that scroll events for navigating up and down were conflicting with the automatic scrolling triggered by focus events. This conflict led to behaviors that diverged from the intended user experience. To address this, I introduced the preventScroll option to give developers more control over the focus-scrolling behavior. This would allow developers to decide whether they want to handle scrolling through custom scroll events or allow the automatic focus-based scrolling. If you have any more questions or need further clarification, feel free to ask. Thank you for considering my PR. |
@nakjun12 Could you please let us know what is the detail background scenario? We're working with a lot of media app developers and normally the issue is not a scrolling itself but scrolling distance so devs (at least we know) resolve their issue with Spotlight container. I'm not sure that your issue is the same as those, so it would be very helpful to understand what is your app's situation if you let us know. We may accept your idea if the scenario is general and the issue is inevitable, and we're asking you about scenario to determine whether the new API would mislead app developers to write a kind of undesired pattern. I believe that you understand framework maintainers take care of app patterns seriously. |
@0x64 Thank you for reading my comments, and I understand that you're managing Spotlight carefully. While developing an app with a UI similar to Netflix, I discovered conflicts between I addressed these issues with the preventScroll option, but this resulted in a more complex codebase as I had to control focus directly without spotlight.focus. I aim to allow for more precise control over focus and scrolling behavior, even directly within spotlight.focus. One concern I have is that my PR addresses both spotlight.focus and focusElement. If during the review process it's determined that addressing both in a single PR complicates the review, I'm open to considering splitting it into separate PRs. Thank you once again for considering my PR. If you have any further questions or need clarification, please don't hesitate to ask. |
@nakjun12 Sorry for late response. What is |
@nakjun12 Thanks for your detailed explanation. It sounds makes sense. I can't tell your PR will be merged immediately due to our version maintenance schedule, but we will definitely merge your PR when we start the next version(It contains new APIs, refactoring, API changes etc.) |
@nakjun12 If you don't mind, could we update your branch when we start the next version? We just reviewed briefly, and we may need to change API after in-depth review regarding API consistency. We don't want to make another branch for your suggestion, so we will update your branch directly in this pull request if you agree. |
@0x64 Of course, feel free to update my branch. |
Enact-DCO-1.0-Signed-off-by: Nakjun Hwang [email protected]
Checklist
Issue Resolved / Feature Added
Resolution
Additional Considerations
Separate PRs for focusElement and spotlight.focus
Considering splitting this PR into two: one for focusElement and another for spotlight.focus. This would allow for more focused review and testing of each feature.
Links
Focus with and without scrolling
Comments
To avoid affecting existing code:
A 4th argument was introduced in focusElement specifically for the preventScroll parameter.
For spotlight.focus, preventScroll was added to the existing containerOption.
Extensibility
The preventScroll parameter is designed for integration into other methods using focusElement.