solymosi / npu

Neptun PowerUp! - Felturbózza a Neptun-odat
MIT License
274 stars 47 forks source link

Don't forcefully hide header with CSS #73

Closed Balint66 closed 1 year ago

Balint66 commented 2 years ago

There is a "button" under the header, to hide it. Maybe NPU could make it more visible so nobody could miss it.

It should solve #67

boapps commented 2 years ago

Hey @FeaXR thanks for your work, but I don't think #75 fixes this issue, all that's changed for me is that the button for showing/hiding the header is visible, but the header will be hidden again on page refresh even if I enable it.

What I don't get is why NPU would even touch the header. Maybe in the past there was no built-in way to hide it (?), but now there is, so I see no use for this feature. I think you should just remove all code related to hiding the header.

I have never used node, but I could do a PR if somebody from the project approves this change.

solymosi commented 2 years ago

@boapps In the past when this feature was introduced there was no way to hide the header. Apparently the developers of Neptun have finally realized after ~10 years that this was a sensible feature to have and added it themselves.

I have a few concerns about @FeaXR's PR myself which I will address in my review soon.

FeaXR commented 2 years ago

@boapps The point of hiding the header is to free up half the screen, that is basically wasted by a non-fuctional element By changing how the hiding works I added the possibility to toggle in the header manually if the user sees fit

boapps commented 2 years ago

@solymosi

In the past when this feature was introduced there was no way to hide the header

makes sense, thanks for the info

@FeaXR

By changing how the hiding works I added the possibility to toggle in the header manually

Neptun can do that already and that actually works, while I've found no way to enable the header persistently with your PR

solymosi commented 2 years ago

We should just remove the hiding functionality altogether. What we could leave behind is code that hides it a single time but then leaves things alone, so if the user wants to unhide it, they can. This would ensure continuity in behavior from previous versions of NPU for existing users.

FeaXR commented 2 years ago

@solymosi I'll close my PR #75 then, and when I have time I'll get the new mode of hiding only once working, then I'll open a new PR. The changes in #75 that are in #74 can be still included if you se fit, so only the changes to the hiding method can be disregarded

@boapps Thanks for the correction, I didn't now about the closing button at all, but this new way of hiding only once seems like a good solution to me.

Balint66 commented 2 years ago

I think solymosi's solution would be the best way to do it. Neptun uses a ShowHeader(buttonObject) on the button, that actually hides the PageMethods.SaveHeaderState(visibility). In my opinion, on first login, we could use either of those functions to hide the header, then let Neptun and the user handle the things.

Another thing, that the sidebar has the same functionality, and when it's hidden, on pages that are actually pretty small (like Grade average) the website becomes very compact. Maybe on this the same thing could be applied like on the header.

solymosi commented 1 year ago

Closing as this will be addressed in the next release of NPU.