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: add pin property #205

Merged
merged 1 commit into from
Oct 10, 2021
Merged

feat: add pin property #205

merged 1 commit into from
Oct 10, 2021

Conversation

lunsdorf
Copy link
Contributor

@lunsdorf lunsdorf commented Sep 21, 2021

Implements issues #4, although I named the property pin instead of fixed since everything related is named like that.

This PR applies all changes from #131 as a patch together with an additional fix for the unit tests. I did it this way, because I deleted to fork I worked on years ago and couldn't figure out how to append to an existing PR with all sources deleted.

I did this because people keep asking if the changes could be integrated. I did not re-evaluate or actually test the changes I made when I originally wrote this. So somebody should definitly do that.

@lunsdorf lunsdorf mentioned this pull request Sep 21, 2021
@janczizikow
Copy link
Collaborator

Hey @lunsdorf thanks for the PR! Looks like a nice thing to add! I'll try to have a closer look at it this week. Meanwhile I had a question that came to me while glancing at it initially:

What if the pin property changes once the Headroom has been already rendered? I think there might be some additional logic that should be added either in componentDidUpdate or getDerivedStateFromProps.

Also would be great if you could mention the prop in the docs (www/pages/index.md)

@lunsdorf
Copy link
Contributor Author

Ok, I added the prop in the docs and also check for pin property changes in componentDidUpdate. I'm reusing the scroll handler function for this, as this is the one that uses requestAnimationFrame callback. The naming is a bit unfortunate however.

src/index.js Outdated
Comment on lines 153 to 156
} else if (prevProps.pin !== this.props.pin) {
this.handleScroll()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there any reason to put it in else if instead of if? Previous conditions check if props changed from disabled: true to disabled: false or the other way around. Those two cases are handled differently but it's unrelated to change of pin prop. So I think you could change it to a separate if instead of else if.

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 think I simply added it the the block I saw without any particular reason :) I changed it to a separate if.

Copy link
Collaborator

@janczizikow janczizikow left a comment

Choose a reason for hiding this comment

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

LGTM 🎉

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