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

Migrate from old BottomNavigationBar to NavigationBar #783

Merged
merged 4 commits into from
Oct 4, 2023

Conversation

ggichure
Copy link
Collaborator

Pull Request Description

This PR migrates from old BottomNavigationBar to NavigationBar

Issue Being Fixed

Issue Number: #764

Screenshots / Recordings

Screenshot_1695980373

Checklist

  • Did you update CHANGELOG.md?
  • Did you use localized strings where applicable?
  • Did you add semanticLabels where applicable for accessibility?

@hjiangsu
Copy link
Member

hjiangsu commented Oct 2, 2023

Hey @ggichure, thanks for the PR!

Unfortunately, with some of the refactor that has been taking place surrounding the feed page, the bottom nav bar code was moved elsewhere. The bottom nav bar code is now in lib/thunder/widgets/bottom_nav_bar. It would be appreciated if you could make the changes to the new file if its not too much work 😅

@ggichure
Copy link
Collaborator Author

ggichure commented Oct 2, 2023

Hey @ggichure, thanks for the PR!

Unfortunately, with some of the refactor that has been taking place surrounding the feed page, the bottom nav bar code was moved elsewhere. The bottom nav bar code is now in lib/thunder/widgets/bottom_nav_bar. It would be appreciated if you could make the changes to the new file if its not too much work 😅

Will do and submit a PR by EOD tomorrow.

@hjiangsu
Copy link
Member

hjiangsu commented Oct 2, 2023

Thanks, I appreciate it! Sorry again for this 😅

@ggichure
Copy link
Collaborator Author

ggichure commented Oct 3, 2023

@hjiangsu I force pushed when rebasing and it closed the PR. Please reopen it, else I can create a new PR.

@hjiangsu hjiangsu reopened this Oct 3, 2023
@hjiangsu
Copy link
Member

hjiangsu commented Oct 3, 2023

Reopened! Let me know if its all good on your end

@ggichure
Copy link
Collaborator Author

ggichure commented Oct 3, 2023

Screenshot 2023-10-03 at 22 52 36

Reopened! Let me know if its all good on your end

All good. Ready for review.

Copy link
Member

@hjiangsu hjiangsu left a comment

Choose a reason for hiding this comment

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

LGTM!

@hjiangsu hjiangsu merged commit c57ac7f into thunder-app:develop Oct 4, 2023
1 check passed
@hjiangsu
Copy link
Member

hjiangsu commented Oct 4, 2023

So I'm just running the latest develop build (with this change) and I'm finding the animations a bit weird when navigating through many pages at once (e.g., navigating from feed page to settings page).

I wonder if we can potentially fix that. I'm assuming this is because we still have the option of animating through pages, which is what is triggering this issue.

RPReplay_Final1696435771.mp4

@ggichure
Copy link
Collaborator Author

ggichure commented Oct 4, 2023

So I'm just running the latest develop build (with this change) and I'm finding the animations a bit weird when navigating through many pages at once (e.g., navigating from feed page to settings page).

I wonder if we can potentially fix that. I'm assuming this is because we still have the option of animating through pages, which is what is triggering this issue.
RPReplay_Final1696435771.mp4

I'll have a look.

@micahmo
Copy link
Member

micahmo commented Oct 5, 2023

@hjiangsu Are you talking about how all of the icons are highlighted at the bottom while it scrolls through the pages? Technically the old nav bar did that as well, but since the highlight wasn't as prominent, it wasn't as noticeable.

This goes back to the discussion we had in Matrix about whether we should really be scrolling through all the pages like that. It is kind of cool, but I think it's also in the minority of apps that do something like that. 😊

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.

3 participants