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

Consolidate all navigation together #1707

Merged
merged 4 commits into from
Feb 19, 2025
Merged

Conversation

hjiangsu
Copy link
Member

Pull Request Description

This PR attempts to consolidate all our existing methods of navigating to specific pages (posts/comments/modlog, etc) into one centralized file (navigation.dart). The reason for this is to help reduce the fragmentation of navigation logic, and to hopefully make navigation more consistent.

In addition to this, there are a few more changes including:

  • Updating flex_color_scheme to 8.1.1. This update applies a hotfix that fixes Fix color of block icon on block user button #1693 on a more general basis
  • Updating the settings navigation logic to also include support for sub-pages (e.g., Account -> Discussion Languages, Account -> Media Management, Appearance -> Posts, etc.).
    • This was mainly done to consolidate the settings-related navigation logic, but includes a side benefit where you can directly search for a sub-page (e.g., searching for Manage Media allows you to go straight to the Manage Media page rather than the Account page)

I tested most of the navigation logic, however, since there are so many ways of navigating to different pages, there might be some edge cases that I may have missed! I think the easiest way to know is to merge this in early and daily it to see if we encounter any issues.

There may be some follow up PRs for this to further consolidate and clean up the individual navigation functions!

Issue Being Fixed

Issue Number: N/A

Screenshots / Recordings

Checklist

  • If a new package was added, did you ensure it uses an appropriate license and is actively maintained?
  • Did you use localized strings (and added appropriate descriptions) where applicable?
  • Did you add semanticLabels where applicable for accessibility?

Copy link
Member

@micahmo micahmo left a comment

Choose a reason for hiding this comment

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

LGTM! We definitely had a lot of navigation stuff all over the place, and a lot of copy-pasting. I like the consolidation. I just did a quick review a left a couple small comments, but otherwise I think we can get this in and start testing it.

@@ -43,7 +43,7 @@ class ThemeBloc extends Bloc<ThemeEvent, ThemeState> {
bool usePureBlackTheme = prefs.getBool(LocalSettings.usePureBlackTheme.name) ?? false;
if (usePureBlackTheme && (themeType == ThemeType.dark || (themeType == ThemeType.system && brightness == Brightness.dark))) themeType = ThemeType.pureBlack;

bool useDarkTheme = themeType == ThemeType.dark || themeType == ThemeType.pureBlack;
bool useDarkTheme = themeType == ThemeType.dark || themeType == ThemeType.pureBlack || (themeType == ThemeType.system && brightness == Brightness.dark);
Copy link
Member

Choose a reason for hiding this comment

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

I feel like we make a similar check in a lot of places. Maybe we can pull this out into a helper method too.

Copy link
Member Author

Choose a reason for hiding this comment

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

I just checked the codebase, and pretty much all usage of useDarkTheme references the ThemeBloc so we might not need any helper functions for this.

There was one case where we were calculating the useDarkTheme, but that turned out to be in a widget that we no longer use, so I removed it! See 0c25ca6

@hjiangsu hjiangsu force-pushed the feat/consolidate-navigation branch from d5d80fc to 0fe0aa2 Compare February 19, 2025 17:42
@hjiangsu hjiangsu merged commit d987e48 into develop Feb 19, 2025
1 check passed
@hjiangsu hjiangsu deleted the feat/consolidate-navigation branch February 19, 2025 21:00
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.

2 participants