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

nav: slide #761

Merged
merged 10 commits into from
Sep 6, 2022
Merged

nav: slide #761

merged 10 commits into from
Sep 6, 2022

Conversation

rcrdlbl
Copy link
Contributor

@rcrdlbl rcrdlbl commented Sep 1, 2022

I chose react-spring for the animation library as there's two major actively developed NPM packages for spring-based animations, and the other one (framer-motion) requires react 18+

i was originally trying to be clever and use react-spring's useTransition() hook to respond to the route but decided it's better if the logic behind this is as simple as possible.

Improvements to implement

  • Detecting if the page is being refreshed/first-loaded to avoid triggering the animation on page load
  • Avoiding the flash of the GroupSidebar losing its group upon returning to all groups by preventing it from rerendering until the animation finishes

@vercel
Copy link

vercel bot commented Sep 1, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
chatstead ✅ Ready (Inspect) Visit Preview Sep 6, 2022 at 7:05PM (UTC)
homestead ✅ Ready (Inspect) Visit Preview Sep 6, 2022 at 7:05PM (UTC)
homestead-storybook ✅ Ready (Inspect) Visit Preview Sep 6, 2022 at 7:05PM (UTC)

@vercel vercel bot temporarily deployed to Preview – homestead September 1, 2022 03:28 Inactive
@vercel vercel bot temporarily deployed to Preview – homestead-storybook September 1, 2022 03:29 Inactive
@vercel vercel bot temporarily deployed to Preview – chatstead September 1, 2022 03:30 Inactive
urcades
urcades previously requested changes Sep 1, 2022
Copy link
Member

@arthyn arthyn left a comment

Choose a reason for hiding this comment

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

LGTM after initial run fix

@vercel vercel bot temporarily deployed to Preview – homestead September 4, 2022 00:41 Inactive
@vercel vercel bot temporarily deployed to Preview – chatstead September 4, 2022 00:42 Inactive
@vercel vercel bot temporarily deployed to Preview – homestead-storybook September 4, 2022 00:44 Inactive
@rcrdlbl rcrdlbl requested a review from urcades September 4, 2022 00:51
@rcrdlbl
Copy link
Contributor Author

rcrdlbl commented Sep 4, 2022

alright – I realized that react-spring perhaps isn't the best library for this task, and i've switched to the latest version of framer-motion that supports react 17 – framer-motion's documentation is far better, the resulting code is easier to read, requires less massaging of app state into a library-friendly form, and the spring physics values it uses actually match up with the ones that figma exports.

@urcades these animations look better?

I've set the initial animation to not occur on mount, but when the nav's in single-group view, it does still have a slight animate-in when it loads.

I think this is because for some reason, when the main nav loads, the Nav component renders once, but when in single-group view, it mounts then re-renders, and due to it no longer being the first render, the animation triggers.

I'm not sure what exactly can be done to prevent this, if anyone has an idea let me know.

@jamesacklin
Copy link
Member

Try this:

  • derive a prop from useParams
  • pass the prop to the nav element in the view
  • trigger the hook based on that prop (shouldAnimate or something like that)

In classic PM style, I didn't read your code and you might have already attempted this. If the issue is with first load/mount based on route, that's where I would look first.

@vercel vercel bot temporarily deployed to Preview – chatstead September 6, 2022 16:04 Inactive
@vercel vercel bot temporarily deployed to Preview – homestead-storybook September 6, 2022 16:06 Inactive
@vercel vercel bot temporarily deployed to Preview – homestead September 6, 2022 16:08 Inactive
@rcrdlbl
Copy link
Contributor Author

rcrdlbl commented Sep 6, 2022

@arthyn okay so – with this commit, when in single-group view, upon refresh/URL navigation the sidebar simply doesn't load in lol

@rcrdlbl
Copy link
Contributor Author

rcrdlbl commented Sep 6, 2022

Screen Shot 2022-09-06 at 12 59 08 PM

I think this logic is the problem –– i don't know what we're using the 'hidden' state for but it's not that, there's seperate navs for mobile that are already 'hidden'

@jamesacklin
Copy link
Member

@rcrdlbl We used "hidden" in the mobile view - navigate into the group, the group nav should be shown; navigate into a channel, the group nav should be hidden but retrievable.

We're not showing mobile for the demo other than Pocket so feel free to TODO patch around this.

@rcrdlbl
Copy link
Contributor Author

rcrdlbl commented Sep 6, 2022

Alright, i've cut it up – it should be good now

@vercel vercel bot temporarily deployed to Preview – homestead-storybook September 6, 2022 17:48 Inactive
@rcrdlbl
Copy link
Contributor Author

rcrdlbl commented Sep 6, 2022

@rcrdlbl We used "hidden" in the mobile view - navigate into the group, the group nav should be shown; navigate into a channel, the group nav should be hidden but retrievable.

We're not showing mobile for the demo other than Pocket so feel free to TODO patch around this.

It seems like if i remove the "hidden" logic from the nav stuff it still works this way? You return to the nav by clicking the little back button next to the group name that shows up in the mobile view, correct?

@vercel vercel bot temporarily deployed to Preview – chatstead September 6, 2022 17:49 Inactive
@vercel vercel bot temporarily deployed to Preview – homestead September 6, 2022 17:50 Inactive
@jamesacklin
Copy link
Member

clicking the little back button next to the group name that shows up in the mobile view

I did this to force a route change to get around this problem, yes.

Comment on lines 31 to 52
const flag = useRouteGroup();
const inChannel = useMatch('/groups/:ship/:name/channels*');
const firstRender = useIsFirstRender();
const animationConfig = {
type: 'spring',
stiffness: 2880,
damping: 120,
};

useEffect(() => {
if (!firstRender) {
return;
}

if (isChat) {
useNavStore.getState().navigatePrimary('dm');
} else if (flag) {
useNavStore.getState().navigatePrimary('group', flag);
} else {
useNavStore.getState().navigatePrimary('main');
}
}, [flag, firstRender, isMobile, isChat, inChannel]);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const flag = useRouteGroup();
const inChannel = useMatch('/groups/:ship/:name/channels*');
const firstRender = useIsFirstRender();
const animationConfig = {
type: 'spring',
stiffness: 2880,
damping: 120,
};
useEffect(() => {
if (!firstRender) {
return;
}
if (isChat) {
useNavStore.getState().navigatePrimary('dm');
} else if (flag) {
useNavStore.getState().navigatePrimary('group', flag);
} else {
useNavStore.getState().navigatePrimary('main');
}
}, [flag, firstRender, isMobile, isChat, inChannel]);
const flag = useRouteGroup();
const inChannel = useMatch('/groups/:ship/:name/channels*');
const animationConfig = {
type: 'spring',
stiffness: 2880,
damping: 120,
};
useEffect(() => {
if (isChat) {
useNavStore.getState().navigatePrimary('dm');
} else if (flag && isMobile && inChannel) {
useNavStore.getState().navigatePrimary('hidden');
} else if (flag) {
useNavStore.getState().navigatePrimary('group', flag);
} else {
useNavStore.getState().navigatePrimary('main');
}
}, [flag, isMobile, isChat, inChannel]);

Copy link
Member

Choose a reason for hiding this comment

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

went back and took a second look at the logic. we still need the hidden behavior if you enter a page in a channel. this also corrects the bad logic from earlier

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just pushed a change that actually seems to get 'hidden' set properly on mobile

@vercel vercel bot temporarily deployed to Preview – homestead-storybook September 6, 2022 18:52 Inactive
@vercel vercel bot temporarily deployed to Preview – chatstead September 6, 2022 18:53 Inactive
@vercel vercel bot temporarily deployed to Preview – homestead September 6, 2022 18:53 Inactive
@vercel vercel bot temporarily deployed to Preview – chatstead September 6, 2022 18:55 Inactive
@vercel vercel bot temporarily deployed to Preview – homestead September 6, 2022 18:56 Inactive
@arthyn arthyn self-requested a review September 6, 2022 18:57
Copy link
Member

@arthyn arthyn left a comment

Choose a reason for hiding this comment

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

LGTM!

@vercel vercel bot temporarily deployed to Preview – homestead-storybook September 6, 2022 18:58 Inactive
@vercel vercel bot temporarily deployed to Preview – homestead-storybook September 6, 2022 19:03 Inactive
@vercel vercel bot temporarily deployed to Preview – homestead September 6, 2022 19:04 Inactive
@vercel vercel bot temporarily deployed to Preview – chatstead September 6, 2022 19:05 Inactive
@rcrdlbl rcrdlbl merged commit 423eb6e into master Sep 6, 2022
@rcrdlbl rcrdlbl deleted the dd/slide branch September 6, 2022 19:06
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