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.

Sign up for GitHub

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

Jump to bottom

feat: carousel integration with history navigation #1601

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from
Draft

feat: carousel integration with history navigation #1601

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8745e4a
feat: carousel integration with history navigation
aegroto Nov 18, 2024
4239f9a
Merge branch 'master' of github.com:stackernews/stacker.news into enh…
aegroto Nov 21, 2024
ec2ab58
Merge branch 'master' into enhancement-backward-carousel-images
huumn Nov 21, 2024
c691259
Merge branch 'master' of github.com:stackernews/stacker.news into enh…
aegroto Nov 21, 2024
a7a16e4
Merge branch 'enhancement-backward-carousel-images' of github.com:aeg…
aegroto Nov 21, 2024
3be1418
fix: navigation-based carousel close action
aegroto Nov 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion components/carousel.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import ArrowRight from '@/svgs/arrow-right-line.svg'
import styles from './carousel.module.css'
import { useShowModal } from './modal'
import { Dropdown } from 'react-bootstrap'
import { usePathname } from 'next/navigation'

function useSwiping ({ moveLeft, moveRight }) {
const [touchStartX, setTouchStartX] = useState(null)
Expand Down Expand Up @@ -104,12 +105,19 @@ function CarouselOverflow ({ originalSrc, rel }) {
}

export function CarouselProvider ({ children }) {
const pathname = usePathname()

const media = useRef(new Map())
const showModal = useShowModal()

const showCarousel = useCallback(({ src }) => {
window.history.pushState(null, '', pathname)
Copy link
Member

@ekzyis ekzyis Nov 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't a shallow push. On backwards navigation, this closes the carousel by reloading the page. We do not want to reload the page, we only want to close the carousel on backwards navigation.

Mhh, on closer inspection, maybe this doesn't trigger a page reload on backward navigation so this could be fine. However, I am still curious why you couldn't use next/router.

I wasn't able to use router.push as suggested in the original issue

Why not?

You also need to pop the state again on close since else, manually closing an image will mean that your next backward navigation will do nothing except popping that stale state.

So if I open many images and close all of them myself, I need to click back as many times as I opened images before I actually go back.

Copy link
Contributor Author

@aegroto aegroto Nov 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have not been able to use router.push because it was not updating the history anyway. I don't know the reason, but I believe it may be related to the path being identical to the last one, and I guess the router drops duplicate entries. An alternative would be to use a different URL, but this hinders the modal to be opened, I still have to investigate this behaviour.

I didn't understand that the push had to be unique for the entire carousel. I will work on this as soon as possible.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding a query param might work:

router.push({ pathname: router.pathname, query: { carousel: true, ...router.query}}, router.asPath, { shallow: true }}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's what I've been trying to do but it looks like the carousel is immediately closed after a 'router.push' or 'router.replace' call. This does not happen when calling native `window' APIs though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you used shallow: true and it did that, then I'd guess something is rerendering that shouldn't.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this is due to line 73 of modal.js:

router.events.on('routeChangeStart', maybeOnClose)

The push call triggers this event, which then calls maybeOnClose. This makes sense as the native window API is not trigger router's events.


showModal((close, setOptions) => {
return <Carousel close={close} mediaArr={Array.from(media.current.entries())} src={src} setOptions={setOptions} />
const closeAction = () => {
window.history.back()
}
return <Carousel close={closeAction} mediaArr={Array.from(media.current.entries())} src={src} setOptions={setOptions} />
}, {
fullScreen: true,
overflow: <CarouselOverflow {...media.current.get(src)} />
Expand Down
Loading