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

Add Collapse Animation To LatestReadme #3523

Merged
merged 2 commits into from Feb 7, 2023
Merged

Conversation

AdrianAndersen
Copy link
Member

@AdrianAndersen AdrianAndersen commented Feb 6, 2023

Description

  • refactor(LatestReadme): class component => functional
  • feat(LatestReadme): animated collapse

Use the Collapse element from "react-select" to animate the LatestReadme component. This makes it much smoother to look at, as it does not jump from collapsed to expanded.

Result

Before

Screen.Recording.2023-02-06.at.15.24.41.mov

After

Screen.Recording.2023-02-06.at.15.22.42.mov

Testing

  • I have thoroughly tested my changes.

Resolves ABA-251

@AdrianAndersen AdrianAndersen added enhancement Pull requests that make enhancements, instead of just purely new features review-needed Pull requests that need review small-fix Pull requests that fix something small labels Feb 6, 2023
@AdrianAndersen AdrianAndersen requested a review from a team February 6, 2023 14:27
@AdrianAndersen AdrianAndersen self-assigned this Feb 6, 2023
@linear
Copy link

linear bot commented Feb 6, 2023

Copy link
Member

@ivarnakken ivarnakken left a comment

Choose a reason for hiding this comment

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

I'm not a fan of using third-party libraries for this as the animation feels off (especially considering we just standardized them). It does not conform with our animations and it is slow.

However, this is definitely better than nothing 🤩. But we should write up a todo for removing react-collapse (it's only used one other place).

@ivarnakken ivarnakken added approved Pull requests that have been approved and removed review-needed Pull requests that need review labels Feb 6, 2023
Comment on lines +19 to +21
useEffect(() => {
setExpanded(expandedInitially);
}, [expandedInitially]);
Copy link
Member

Choose a reason for hiding this comment

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

Do you need this useEffect? In my mind it has no effect 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

In prod, it will not have any effect, since all the fetching is done server side before it reaches the client. However, when running this locally, the value changes as the fetching is happening. And for some reason, the component does not re render when the initial value changes as the fetches happen. Therefore, the initial value ends up wrong, if you do not include this.

Copy link
Contributor

@ollfkaih ollfkaih left a comment

Choose a reason for hiding this comment

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

LGTM

@AdrianAndersen AdrianAndersen merged commit 798f4c8 into master Feb 7, 2023
@AdrianAndersen AdrianAndersen deleted the animatedReadmes branch February 7, 2023 10:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Pull requests that have been approved enhancement Pull requests that make enhancements, instead of just purely new features small-fix Pull requests that fix something small
Projects
None yet
4 participants