Skip to content

fix: scroll-anims #61

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Conversation

ArcherTheReal
Copy link
Contributor

I fixed the visual bugs mentioned by rudko and also added a toggle at the bottom

@PancakeTAS
Copy link
Contributor

I am against the idea of adding an extra toggle for this.. If one even has a reason to want to turn them off, then the effect is already too aggressive. Instead, this should tie into power saving probably

@ArcherTheReal
Copy link
Contributor Author

@p2r3 said to add a toggle for it, but i can easily merge it with the other one if needed

@p2r3
Copy link
Owner

p2r3 commented Jul 26, 2024

There's no reason not to give people this customization option, especially not if it's out of principle. It doesn't cost us anything to add an extra toggle.

@soni801
Copy link
Collaborator

soni801 commented Jul 26, 2024

I agree with pancake here. There shouldn't be any reason for people to want to disable this, other than potentially power savings. The only thing we're gonna achieve by adding a toggle is making the site more cluttered.

@p2r3
Copy link
Owner

p2r3 commented Jul 26, 2024

The clutter in question is a thin toggle at the bottom of the page, but I yield. If it's 2 to 1, then do just rename the existing toggle and bundle this with it.

@ArcherTheReal
Copy link
Contributor Author

We can also make a dropdown at the bottom, so its a text called settings and clicking it makes all settings pop out and back in, in case we add more later, although this isn't necessary for the time being

* @param {HTMLElement} element
* @returns {boolean}
*/
function elementFilter(element){
Copy link
Owner

Choose a reason for hiding this comment

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

Style nitpick - this code base typically adds a space between the function name and the brackets.

function elementFilter(element){
let ret = true;
ret = ret && element
ret = ret && !element.classList.contains('nofade');
Copy link
Owner

Choose a reason for hiding this comment

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

Style nitpick - this code base avoids single tick quotes for strings whenever possible.

Copy link
Owner

Choose a reason for hiding this comment

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

Remove and use existing toggle as discussed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So i should add this to the powersaving?

Copy link
Owner

Choose a reason for hiding this comment

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

Yes, and rename the toggle. As discussed.

@PancakeTAS
Copy link
Contributor

stale?

@ArcherTheReal
Copy link
Contributor Author

Cant work on this atm

@p2r3 p2r3 marked this pull request as draft August 25, 2024 15:46
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.

4 participants