search

WickyNilliams / headroom.js

Give your pages some headroom. Hide your header until you need it
https://wicky.nillia.ms/headroom.js/
MIT License
10.86k stars 824 forks source link

Breaking change with no class in quotations #359

Closed mav2287 closed 4 years ago

mav2287 commented 4 years ago

In previous versions you could set a class to empty for example we had several projects with initial : "", so that it wouldn't add an initial class. With the new changes for multiple classes that throws an error and prevents headroom from running. We had done this as we didn't want additional classes we didn't need added to things.

WickyNilliams commented 4 years ago

It seems it's been "broken" for what you want since v10.2.0 (released Sept 2019), not since the change for multiple classes.

But I'm not sure I want to support this, it was never an intended feature. Why do you want it? It doesn't save any work within headroom, all the code still runs etc.

mav2287 commented 4 years ago

TLDR; It looks like I'm not the first to request it and it would be really easy to re-implement as this was an un-intention breaking change.

I am a huge fan of less is more so I alway advocate being able to turn things off. In this case not adding classes becomes especially useful when when you have a CMS or other system that already adds tons of classes to stuff. In a lot of cases you have multiple devs assigning stuff based on class and it can just be easier to not throw another into the mix.

The other side of this coin is that it is an unintentional breaking change. The only reason I caught is that when I updated to the new version ( the site I was working on had 9 ) headroom just stopped working. There are several other projects our company has I would like to update, but I would have to change this in every single one.

WickyNilliams commented 4 years ago

Hey, can you point to any issues you found that also request this? I can't remember any from memory, but it's possible I'm forgetting.

I just tested what happens in v0.9.4 if you pass an empty string for the initial class:

const header = document.querySelector("header");

const hr = new Headroom(header, {
  classes: {
    initial: ""
  }
});
hr.init()

Whilst it doesn't error, it doesn't actually do what you've described - the initial class is added anyway! See here: https://codepen.io/WickyNilliams/pen/rNVOYVW?editors=1010.

So I would advise against this. It never did what you thought and I don't want to support this use-case anyway. This is not just as a matter of taste, but because the internals of the library would make it difficult (it would require a pretty large reworking of how its state is managed)

WickyNilliams commented 4 years ago

Closing, but happy to discuss further

mav2287 commented 4 years ago

I just searched super quick as I am slammed with work today, but here are 2 example of issues with supporters requesting the functionality.

238 - https://github.com/WickyNilliams/headroom.js/issues/238

In this one 2 people the OP and another user ask for it citing the same reasons I did. There is also several thumbs up on it by various other users

226 - https://github.com/WickyNilliams/headroom.js/issues/226

Same request and reasons as above by a different user

As I mentioned before when it comes to classes being added I am a big fan ( as are these people ) of less is more. I work with Wordpress a lot and it just adds classes for fun it seems. It also seems like it would be pretty quick and easy to add support for this.

hacknug commented 4 years ago

As one of the people who brought this up a couple years ago, today I wouldn't bother adding support for it. Like @WickyNilliams said in https://github.com/WickyNilliams/headroom.js/issues/226#issuecomment-223307160, without any real performance issues, I don't think it's worth the effort.

So I would advise against this. It never did what you thought and I don't want to support this use-case anyway. This is not just as a matter of taste, but because the internals of the library would make it difficult (it would require a pretty large reworking of how its state is managed)

If you wanna take a stab at it and open a PR, I'll try to review and test the changes but again, please make sure it makes sense to spend time and resources into this.

As I mentioned before when it comes to classes being added I am a big fan ( as are these people ) of less is more. I work with Wordpress a lot and it just adds classes for fun it seems.

I agree with you but you need to keep in mind you should only remove useless stuff. The classes we're talking about are used to keep track of the instance's state (unlike WordPress which in my cases are useless 99% of the time).

It also seems like it would be pretty quick and easy to add support for this.

The creator of the library literally told you it would require to rewrite the whole state-management part of the library. Make sure you keep this in mind if you end up trying to change the way the library works :+1:

WickyNilliams commented 4 years ago

I was going to ping you to ask your opinion @hacknug, since I saw you had commented on a previous issue. Interesting that you've changed your mind since.

I'm not averse to this idea in principle, but as I said it would require a fairly substantial rewrite of the internals. I plan to do this eventually, but not right now. I will come back to this then