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(parallax): child horizontal prop defaults to parent's #1458

Merged
merged 4 commits into from
Apr 25, 2021

Conversation

kindoflew
Copy link
Collaborator

Why

Currently, when Parallax has horizontal={true}, all ParallaxLayers still translate vertically unless horizontal={true} is manually set on all of them. This change makes it so ParallaxLayers' default horizontal matches Parallax's.

What

  • Added horizontal to IParallax interface and Parallax's state
  • Added a check in ParallaxLayer to see if horizontal === undefined and reassign it if it is.
  • Removed a CSS rule in sandbox demo that was causing numbers to appear too far to the left.

Checklist

  • Documentation updated: (Pending this being merged)
  • Demo added: n/a
  • Ready to be merged

You can test this by looking at the sandbox demo "Parallax" (not vert). Currently, when you scroll horizontally, the layers still translate vertically.

This might be a breaking change if anyone is relying on this behavior, but it wouldn't affect anyone who has already manually set horizontal on ParallaxLayers.

I will also submit a PR for the docs to add horizontal to ParallaxLayer's prop list, but will wait until a decision is reached on this so I can accurately describe its behavior. I understand everyone is probably busy with v9 bug fixes, so if this is not a high priority right now, I'll just update the docs to reflect how it currently works.

@codesandbox-ci
Copy link

codesandbox-ci bot commented Apr 24, 2021

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit e988e4f:

Sandbox Source
spring-card Configuration
spring-goo-blobs Configuration
spring-flip-card Configuration
spring-slide Configuration
spring-draggable-list Configuration
spring-cards-stack Configuration
spring-viewpager Configuration
spring-simple-transition Configuration
spring-image-fade Configuration
spring-list-reordering Configuration

@kindoflew
Copy link
Collaborator Author

I decided reassigning a prop was probably a smell, so I refactored it to be a new const that replaces horizontal conditionals in ParallaxLayer. All other changes are the same.

Copy link
Member

@joshuaellis joshuaellis left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution!! ⭐ I had thought the demo looked odd when I added it...

@joshuaellis
Copy link
Member

Regarding your PR notes, i think this makes perfect sense imo, as long as we clearly state this is in the docs it'll be fine, i'll also update the changelogs to reflect this POTENTIALLY_BREAKING_CHANGE.

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