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

fix(react): support SwiperSlider components wrapped in higher order components and fix nested fragments bug #4144

Merged
merged 5 commits into from
Feb 1, 2021

Conversation

tomjn
Copy link
Contributor

@tomjn tomjn commented Jan 26, 2021

Support SwiperSlide components wrapped in higher order components and fix a nested fragments bug

This change does several things:

  • Keeps the requirement that Swiper Slides must be SwiperSlide components
  • Fixes a bug where a fragment containing multiple fragments only processed slides in the first fragment
  • Fixes not detecting SwiperSlide components when they're wrapped by a HOC by middleware

For example, this slider only has 2 slides:

<Swiper>
	<React.Fragment>
		<React.Fragment>
			<SwiperSlide>Slide 1</SwiperSlide>
			<SwiperSlide>Slide 2</SwiperSlide>
		</React.Fragment>
		<React.Fragment>
			<SwiperSlide>Slide 3</SwiperSlide>
			<SwiperSlide>Slide 4</SwiperSlide>
		</React.Fragment>
	</React.Fragment>
</Swiper>

This slider might not work at all:

const ImportantSlides = () => (
	<React.Fragment>
		<SwiperSlide>Slide 1</SwiperSlide>
		<SwiperSlide>Slide 2</SwiperSlide>
	</React.Fragment>
);

const SecondarySlides = () => (
	<React.Fragment>
		<SwiperSlide>Slide 3</SwiperSlide>
		<SwiperSlide>Slide 4</SwiperSlide>
	</React.Fragment>
);

export default () => {
	return (
		<Swiper>
			<ImportantSlides/>
			<SecondarySlides/>
		</Swiper>
	);
};

The reason for this is that I'm trying to help someone who is trying to build a an editor UI for Swiper in a CMS, but the recommended workaround given here is impossible.

I traced the introduction of the code in question to the addition of virtual slides in v6 alpha 4 but could not find any reason for the requirement other than that without it the code could not detect the slide. It appears to be a technical limitation. This PR corrects that by introducing proper recursion while preserving the requirement that the slide be used.

While investigating I also uncovered the Fragment bug. The checking of the component name also means that components that just return SwiperSlide components perhaps for organisational purposes no longer work.

This also prevents libraries such as slot fills from working with Swiper. For example a React application may provide a Slot inside a Swiper library, then use a Fill elsewhere to insert an additional SwiperSlide component.

…d fix nested fragments bug

This change does several things:

 - Keeps the requirement that Swiper Slides must be `SwiperSlide` components
 - Fixes a possible edge case where a fragment containing multiple fragments only processed slides in the first fragment
 - Fixes not detecting `SwiperSlide` components when they're wrapped by a HOC by middleware
@nolimits4web
Copy link
Owner

Thanks, checked and now this breaks none SwiperSlide components, e.g.:

      <Swiper>
        <SwiperSlide>Slide 1</SwiperSlide>
        <SwiperSlide>Slide 2</SwiperSlide>
        <SwiperSlide>Slide 3</SwiperSlide>
        <SwiperSlide>Slide 4</SwiperSlide>
        <SwiperSlide>Slide 5</SwiperSlide>
        <div>Hello World</div>
      </Swiper>

will throw an error

@tomjn
Copy link
Contributor Author

tomjn commented Jan 26, 2021

hmmm I think I see why that happens, I'll post an update later today. What I plan to do is make it return the slides, that way if processChildren returns nothing then we know it should go into the end slot. I'll do some testing, and see why the GH Actions didn't catch that either

… descendents that don't have SwiperSlide components
@tomjn
Copy link
Contributor Author

tomjn commented Jan 30, 2021

Just an update that I made changes, but I've been having trouble getting the react playground to run to confirm all is good ( Uncaught Error: Cannot find module './components/core/core-class' ), the playground cannot resolve any of the components in the build folder.

Tests linters etc pass though, and the demos run at least, but I'm unable to confirm these changes work in React via the official tooling

@nolimits4web nolimits4web changed the title Support SwiperSlider components wrapped in higher order components and fix nested fragments bug fix(react): support SwiperSlider components wrapped in higher order components and fix nested fragments bug Feb 1, 2021
@nolimits4web nolimits4web merged commit 9e71974 into nolimits4web:master Feb 1, 2021
@nolimits4web
Copy link
Owner

Merged, thanks!

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.

2 participants