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

feat: carousel back action integrated into browser history #1677

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

aegroto
Copy link
Contributor

@aegroto aegroto commented Dec 3, 2024

Description

This is a cleaner version of this PR as that branch contained a lot of commits and was diverging from master.

Carousel's modal now closes when navigating backward through the browser's history, going back to the post instead of returning to the main page. Fixes #1508.

Screenshots

I have committed a version of the fix which works in case of multiple carousel shows as well:

recording.mp4

Additional Context

I wasn't able to use router.push as suggested in the original issue and had to stick to the native APIs, which is documented by NextJS as an alternative to router methods as well.

The code overrides the close function of the carousel, triggering a 'back' navigation when the carousel is closed. It is still relying on native history APIs, unfortunately a router's shallow navigation immediately closes the carousel, presumably due to the listener I have highlighted in the previous comments.

Before marking this PR ready for review I would like to know if it's fine for your to use window's native APIs, then maybe explore a solution which relies on router in the future.

Checklist

Are your changes backwards compatible? Please answer below: Yes

On a scale of 1-10 how well and how have you QA'd this change and any features it might affect? Please answer below: 8, I tested the changes manually and with various content.

For frontend changes: Tested on mobile, light and dark mode? Please answer below: Yes

Did you introduce any new environment variables? If so, call them out explicitly here: No

@aegroto aegroto marked this pull request as draft December 3, 2024 18:35
@aegroto
Copy link
Contributor Author

aegroto commented Dec 10, 2024

@huumn @ekzyis Sorry for the ping but I'd need a feedback on this PR once you've got some time, just to be aware if this solution relying on the direct APIs is fine for you.

@huumn
Copy link
Member

huumn commented Dec 10, 2024

We are occupied with high priority issues. We will get to this when we have time.

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.

Back/forward browser navigation for carousel images
2 participants