Add applyStyle property

dollarshaveclub / stickybits

Stickybits is a lightweight alternative to `position: sticky` polyfills 🍬

https://github.com/yowainwright/stickybits

MIT License

2.19k stars 167 forks source link

Add applyStyle property #604

Closed mbark closed 4 years ago

mbark commented 4 years ago

This property allows the user to control how they want to apply the styles and classes.

Fixes

Fixes #598

Proposed Changes

Change how styles and classes are applied to use a single function, applyStyle, which users can provide to take control over how to apply them (useful e.g. when a framework like Elm).
Update interface for toggleClasses to take a an object of classes where the key is the class name and the value's truthiness determines if it should be added or removed.
Do only a single call to applyStyle and only when a state changes.

yowainwright commented 4 years ago

@mbark Lots of smart work here. I'm going to take this PR for a test drive before merging. ~Thank you! :pray:

mbark commented 4 years ago

When I switched to for ... in I ran into this bug https://github.com/eslint/eslint/issues/12117 in eslint. It appears that dollarshaveclub/eslint-config-dollarshaveclub is using an older version of eslint that doesn't have the fix for this bug. As a workaround I disabled no-unused-vars for all of the loops and added a comment with a link to the bug as to why I disabled the rule.

You can absolutely add me as contributor :+1:

DanielRuf commented 4 years ago

have the fix for this bug. As a workaround I disabled no-unused-vars for all of the loops and added a comment with a link to the bug as to why I disabled the rule.

I would recommend to use the eslint-disable-next-line comment as we should check the complete rule separately.

mbark commented 4 years ago

@DanielRuf I did use eslint-disable-next-line no-unused-vars for the three affected lines.

I've also squashed all commits together and removed the changes to dist/. Although I'm a bit curious as to why they shouldn't be added if they are checked in? Now the files in dist don't actually represent what is in the src/ which seems confusing to me?

DanielRuf commented 4 years ago

I've also squashed all commits together and removed the changes to dist/. Although I'm a bit curious as to why they shouldn't be added if they are checked in? Now the files in dist don't actually represent what is in the src/ which seems confusing to me?

They should be updated by us when we do a new release. This prevents conflicts with other PRs and branches.

yowainwright commented 4 years ago

@mbark thanks for your great work! I'm a bit hesitant because this update is a big diff (but I think better). I'm going to merge it, run a build, and update the examples before publishing.

Would you like to be a contributor as you did a lot of work here? If something comes up and I have to revert or do a fast-follow, I'd love your help. :pray:

@DanielRuf thanks for your insights and help!

mbark commented 4 years ago

It is definitely quite a big change, I'll start watching this repo to see if there are issues. And if you want to you can add me as contributor @yowainwright .