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

Improve disclosure collapse animation #1704

Merged
merged 1 commit into from
Jul 22, 2023

Conversation

zerebos
Copy link
Contributor

@zerebos zerebos commented Jul 20, 2023

Proposed change

Improves the new collapse feature with a more modern drawer slide animation. It also allows the document to transition the layout of elements at the same time. This results in a much less jarring collapse than before where suddenly the page would shift after the animation completed. This was especially apparent for larger sections.

Please see a demo video blow

GTP06Y7sDA.mp4

This change was tested and works as expected for both mobile and desktop, but I don't have a mobile recording. Others can test on mobile or desktop currently on my personal homepage instance if they want. (URL upon request as I don't want to appear to self-promote)

Closes # (issue)

Type of change

  • New service widget
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Other (please explain)

Minor improvement to a recently introduced feature.

Checklist:

  • If adding a service widget or a change that requires it, I have added a corresponding PR to the documentation here:
  • If adding a new widget I have reviewed the guidelines.
  • If applicable, I have checked that all tests pass with e.g. pnpm lint.
  • If applicable, I have tested my code for new features & regressions on both mobile & desktop devices, using the latest version of major browsers.

@shamoon
Copy link
Collaborator

shamoon commented Jul 20, 2023

Thanks, this is pretty subjective but I've always understood that having more than one animation together creates a more pleasing effect so I've added back the fade. LMK your thoughts.

1704.mov

@shamoon
Copy link
Collaborator

shamoon commented Jul 20, 2023

Did you disable pushing to the PR?

@zerebos
Copy link
Contributor Author

zerebos commented Jul 21, 2023

Hmmm I definitely prefer it without the opacity change personally, but as you said it can be quite subjective. I typically don't see these type of sliding/collapsing animations combined with an opacity transition.

Did you disable pushing to the PR? ("Allow edits by maintainers")

I typically do, yeah, to encourage discussions/suggestions before changes are made and pushed. But I'll turn it back on if it's the norm in this repo.

@shamoon
Copy link
Collaborator

shamoon commented Jul 21, 2023

The thing for me is the group doesnt collapse into a smaller element, it collapses into nothing, so IMHO it looks kinda odd to just slide up to 0px, the fade makes that a bit more graceful.

@shamoon
Copy link
Collaborator

shamoon commented Jul 21, 2023

@benphelps whatever you think here

@benphelps
Copy link
Member

I prefer the opacity fade version myself, but after looking through some more resources on my own, it does seem like the overwhelming consensus is to not fade.

@shamoon shamoon merged commit e4e6bba into gethomepage:main Jul 22, 2023
1 check passed
@zerebos
Copy link
Contributor Author

zerebos commented Jul 22, 2023

I considered suggesting the addition of a new configuration option for type of collapse animation but I think that might be overkill.

For now, I'm going to go look through some discussions and see how else I might contribute.

Copy link
Contributor

github-actions bot commented Feb 5, 2024

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new discussion or issue for related concerns.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 5, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants