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

Consume scroll notification #13

Merged
merged 3 commits into from
Jan 8, 2024

Conversation

pixelshot91
Copy link
Contributor

Enable to nest mulitple ScrollShadow without the inner one interfering with the outer one

Enable to nest mulitple ScrollShadow without the inner one interfering
with the outer one
@RichiB20
Copy link

Hi @pixelshot91, I tried to reproduce your scenario of a ScrollShadow inside another ScrollShadow. However the behavior with and without your change for me is the same. Could you tell me which differences you found?
Thank you

@pixelshot91
Copy link
Contributor Author

I made two video showcasing the bug fix.
before_fix.webm
In this video, scrolling up and down works as expected, but when I scroll in the inner ScrollShadow, I have two huge shadows left and right, but there should not be. There should just be the little shadow left and right for each row.

after_fix.webm
In this video, evrything works as expected

The code used to create the video is available here:
https://github.com/pixelshot91/flutter_scroll_shadow/tree/topic/consume_notification_test
The test is not mergeable yet. I need to add some 'expect' to check the position of the big shadow

@RichiB20
Copy link

Sorry, I tried the two scroll shadow in the same direction and I did't see any problem.
You're right, when you finish the test I will accept your PR.
Thank you for your work

@pixelshot91
Copy link
Contributor Author

I added the test and a 'flutter test' step in the workflow

@pixelshot91
Copy link
Contributor Author

Is ok for you ? @RichiB20

@RichiB20
Copy link

RichiB20 commented Jan 4, 2024

Yes it's ok, as soon as @rickypid is able he will accept the PR.

@rickypid rickypid changed the base branch from master to dev January 8, 2024 19:55
@rickypid
Copy link
Owner

rickypid commented Jan 8, 2024

Hi @pixelshot91,

Sorry for the delay but it's a very intense period,

Thanks to you and @RichiB20 for the contribution.

@rickypid rickypid merged commit c3f216e into rickypid:dev Jan 8, 2024
1 check passed
pixelshot91 added a commit to pixelshot91/booky that referenced this pull request Jan 9, 2024
pixelshot91 added a commit to pixelshot91/booky that referenced this pull request Jan 11, 2024
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