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

feat(router): implement GoRouter navigation system #32

Merged
merged 12 commits into from
Dec 12, 2024

Conversation

phoenixit99
Copy link

  • Setup GoRouter configuration with initial routes • Create app_router.dart with route definitions • Define AppRoute enum for type-safe routing • Configure MaterialApp.router in main.dart

  • Implement core screens with navigation flow • Add SplashScreen with 2-second auto-navigation • Create HomeScreen with basic layout • Setup proper back navigation handling

  • Add comprehensive router tests • Test route paths and names validation • Verify navigation flow from splash to home • Ensure proper widget rendering • Test back navigation behavior

The implementation follows a feature-first structure and includes: • Type-safe route management
• Automatic navigation from splash to home
• Proper handling of back navigation
• Comprehensive test coverage

- Setup GoRouter configuration with initial routes
  • Create app_router.dart with route definitions
  • Define AppRoute enum for type-safe routing
  • Configure MaterialApp.router in main.dart

- Implement core screens with navigation flow
  • Add SplashScreen with 2-second auto-navigation
  • Create HomeScreen with basic layout
  • Setup proper back navigation handling

- Add comprehensive router tests
  • Test route paths and names validation
  • Verify navigation flow from splash to home
  • Ensure proper widget rendering
  • Test back navigation behavior

The implementation follows a feature-first structure and includes:
• Type-safe route management
• Automatic navigation from splash to home
• Proper handling of back navigation
• Comprehensive test coverage
- Apply consistent code formatting
  • Run dart format on all new files
  • Ensure proper indentation
  • Maintain consistent style

This feature provides a foundation for app-wide navigation using
GoRouter, enabling efficient task distribution among team members
while maintaining a cohesive navigation pattern throughout the app.

The implementation follows a feature-first structure and includes:
• Type-safe route management
• Automatic navigation from splash to home
• Proper handling of back navigation
• Comprehensive test coverage
• Consistent code formatting

Closes #23
@esmaeil-ahmadipour esmaeil-ahmadipour added the feature Add/Create new feature label Nov 28, 2024
@esmaeil-ahmadipour esmaeil-ahmadipour linked an issue Nov 28, 2024 that may be closed by this pull request
@esmaeil-ahmadipour esmaeil-ahmadipour marked this pull request as draft November 28, 2024 16:58
@phoenixit99 phoenixit99 marked this pull request as ready for review November 29, 2024 07:29
@esmaeil-ahmadipour esmaeil-ahmadipour marked this pull request as draft November 29, 2024 08:18
@phoenixit99 phoenixit99 marked this pull request as ready for review November 29, 2024 09:09
@esmaeil-ahmadipour
Copy link
Collaborator

esmaeil-ahmadipour commented Nov 29, 2024

@phoenixit99, please amend the commit to resolve the conflict and update the commit message. After making the changes, force-push the updated commit.

Screenshot from 2024-11-29 15-43-24

The highlighted text should be moved to the commit title and follow the Conventional Commits guideline.

conflict: resolved conflicts

approved by esmaeil-ahmadipour
@phoenixit99 phoenixit99 force-pushed the feature/define_routes_for_screens branch from f4e757f to cfd5227 Compare November 29, 2024 14:57
Reorganizes the project structure by moving `app_router` into the inner `src/core` directory to improve modularity and maintainability.

No functionality has been changed in this refactor.
@phoenixit99
Copy link
Author

@phoenixit99, please amend the commit to resolve the conflict and update the commit message. After making the changes, force-push the updated commit.

Screenshot from 2024-11-29 15-43-24

The highlighted text should be moved to the commit title and follow the Conventional Commits guideline.

Thanks @esmaeil-ahmadipour . I updated the commit and restructured the directories

@esmaeil-ahmadipour
Copy link
Collaborator

I had a look at the implementation compared to the Figma design.

The goal for this issue was to create a simple sitemap based on the design, but the current implementation seems incomplete.

All pages were expected to be mocked with navigation between them, but only two pages have been connected to the routing system: a splash page and a home page.

Looking at the design, there isn’t actually a page named "Home," and what’s labeled as "Splash" is more of a "Welcome" page that quickly navigates to the next screen.

Here’s a list of all the main routes we were expecting:

Register Route

  1. SplashScreen
  2. Welcome Page
  3. Initialize Mode
  4. Restoration Seed
  5. Confirmation Seed
  6. Master Password
  7. Validator Config
  8. Initializing
  9. Finish

Basic Route

  1. SplashScreen
  2. Password
  3. Dashboard

@phoenixit99

phoenixit99 added a commit that referenced this pull request Dec 6, 2024
- Add initial route configuration for auth and basic flows
- Configure registration flow routes:
  - Welcome Page
  - Initialize Mode
  - Restoration Seed
  - Confirmation Seed
  - Master Password
  - Validator Config
  - Initializing
  - Finish
- Set up basic flow routes:
  - SplashScreen
  - Password
  - Dashboard

BREAKING CHANGE: New routing architecture implementation
Requires updated navigation handling throughout the app

Resolves: #32
- Add initial route configuration for auth and basic flows
- Configure registration flow routes:
  - Welcome Page
  - Initialize Mode
  - Restoration Seed
  - Confirmation Seed
  - Master Password
  - Validator Config
  - Initializing
  - Finish
- Set up basic flow routes:
  - SplashScreen
  - Password
  - Dashboard

BREAKING CHANGE: New routing architecture implementation
Requires updated navigation handling throughout the app

Resolves: #23
@phoenixit99 phoenixit99 force-pushed the feature/define_routes_for_screens branch from 6b32318 to 03da6c4 Compare December 6, 2024 08:54
@esmaeil-ahmadipour esmaeil-ahmadipour removed the request for review from Ja7ad December 7, 2024 19:03
@phoenixit99
Copy link
Author

I had a look at the implementation compared to the Figma design.

The goal for this issue was to create a simple sitemap based on the design, but the current implementation seems incomplete.

All pages were expected to be mocked with navigation between them, but only two pages have been connected to the routing system: a splash page and a home page.

Looking at the design, there isn’t actually a page named "Home," and what’s labeled as "Splash" is more of a "Welcome" page that quickly navigates to the next screen.

Here’s a list of all the main routes we were expecting:

Register Route

  1. SplashScreen
  2. Welcome Page
  3. Initialize Mode
  4. Restoration Seed
  5. Confirmation Seed
  6. Master Password
  7. Validator Config
  8. Initializing
  9. Finish

Basic Route

  1. SplashScreen
  2. Password
  3. Dashboard

@phoenixit99

Already apply in new PR

@esmaeil-ahmadipour
Copy link
Collaborator

esmaeil-ahmadipour commented Dec 11, 2024

I’ve added 5 comments as a reviewer on certain parts of your code.

Whenever you get a time , please take a look at them directly in the PR so I don’t need to rewrite them for you.

To make things easier for you this time, I’ve summarized all the points here as new comments. please review and take action on them whenever possible.

Let me know once everything is resolved. Thanks so much!


Problems :

#1 :
in this file : lib/presentation/bloc/language_bloc/language_bloc.dart
in this line : 4

const languagePrefsKey = 'languagePrefs';

please if don't use remove it.


#2 :
in this file : lib/src/core/router/app_router.dart
in this part:
all this parts used for register route.
we need another route for basic route.
so create two file one for basic route and another for registration route.
and consume two routes in this file. (in future need move any route parts to self modules.)


#3 :
in this file : lib/src/features/splash/presentation/screen/splash_screen.dart
in this line : 50

                const SizedBox(height: _spacingBetweenElements),

please add Gap package and use it and replace with SizedBox


#4 :
in this file : lib/src/core/router/route_name.dart
in this line : 14 ~ 26

  String get path => switch (this) {
        AppRoute.splash => '/',
        AppRoute.dashboard => '/dashboard',
        AppRoute.password => '/password',
        AppRoute.welcome => '/welcome',
        AppRoute.initializeMode => '/initialize-mode',
        AppRoute.restorationSeed => '/restoration-seed',
        AppRoute.confirmationSeed => '/confirmation-seed',
        AppRoute.masterPassword => '/master-password',
        AppRoute.validatorConfig => '/validator-config',
        AppRoute.initializing => '/initializing',
        AppRoute.finish => '/finish',
      };
      

remove this part and write easy way :)

/// Returns the route path for this enum value.
String get routePath => '/$name';

#5 :
in this file : lib/src/core/router/route_name.dart
in this line : 28~ 40

  String get name => switch (this) {
        AppRoute.splash => 'splash',
        AppRoute.dashboard => 'dashboard',
        AppRoute.password => 'password',
        AppRoute.welcome => 'welcome',
        AppRoute.initializeMode => 'initializeMode',
        AppRoute.restorationSeed => 'restorationSeed',
        AppRoute.confirmationSeed => 'confirmationSeed',
        AppRoute.masterPassword => 'masterPassword',
        AppRoute.validatorConfig => 'validatorConfig',
        AppRoute.initializing => 'initializing',
        AppRoute.finish => 'finish',
      };
      

remove this part and write easy way :)

and for this write :

/// Returns the route name for this enum value.
String get routeName => name;


@phoenixit99

@esmaeil-ahmadipour esmaeil-ahmadipour added the review failed Review not approved, needs changes. label Dec 12, 2024
- Separate to register route and basic route
- Add gap dependency
- remove unused code in language_bloc
- update code in route_name for easy-to-read
@phoenixit99
Copy link
Author

phoenixit99 commented Dec 12, 2024

I’ve added 5 comments as a reviewer on certain parts of your code.

Whenever you get a time , please take a look at them directly in the PR so I don’t need to rewrite them for you.

To make things easier for you this time, I’ve summarized all the points here as new comments. please review and take action on them whenever possible.

Let me know once everything is resolved. Thanks so much!

Problems :

#1 : in this file : lib/presentation/bloc/language_bloc/language_bloc.dart in this line : 4

const languagePrefsKey = 'languagePrefs';

please if don't use remove it.

#2 : in this file : lib/src/core/router/app_router.dart in this part: all this parts used for register route. we need another route for basic route. so create two file one for basic route and another for registration route. and consume two routes in this file. (in future need move any route parts to self modules.)

#3 : in this file : lib/src/features/splash/presentation/screen/splash_screen.dart in this line : 50

                const SizedBox(height: _spacingBetweenElements),

please add Gap package and use it and replace with SizedBox

#4 : in this file : lib/src/core/router/route_name.dart in this line : 14 ~ 26

  String get path => switch (this) {
        AppRoute.splash => '/',
        AppRoute.dashboard => '/dashboard',
        AppRoute.password => '/password',
        AppRoute.welcome => '/welcome',
        AppRoute.initializeMode => '/initialize-mode',
        AppRoute.restorationSeed => '/restoration-seed',
        AppRoute.confirmationSeed => '/confirmation-seed',
        AppRoute.masterPassword => '/master-password',
        AppRoute.validatorConfig => '/validator-config',
        AppRoute.initializing => '/initializing',
        AppRoute.finish => '/finish',
      };
      

remove this part and write easy way :)

/// Returns the route path for this enum value.
String get routePath => '/$name';

#5 : in this file : lib/src/core/router/route_name.dart in this line : 28~ 40

  String get name => switch (this) {
        AppRoute.splash => 'splash',
        AppRoute.dashboard => 'dashboard',
        AppRoute.password => 'password',
        AppRoute.welcome => 'welcome',
        AppRoute.initializeMode => 'initializeMode',
        AppRoute.restorationSeed => 'restorationSeed',
        AppRoute.confirmationSeed => 'confirmationSeed',
        AppRoute.masterPassword => 'masterPassword',
        AppRoute.validatorConfig => 'validatorConfig',
        AppRoute.initializing => 'initializing',
        AppRoute.finish => 'finish',
      };
      

remove this part and write easy way :)

and for this write :

/// Returns the route name for this enum value.
String get routeName => name;

@phoenixit99

Thanks for your comment, I update the code that resolved all feedback

@esmaeil-ahmadipour
Copy link
Collaborator

@phoenixit99 , henry excellent. good job!

Please add a button to all screens to navigate forward through the flow until the end. Make sure users can go back to previous screens, and ensure the last screen (when navigating back) is the first screen of the route, not the splash screen.

  • registrationRoutes: The first screen should be Welcome.
  • basicRoutes: The first screen should be BasicPassword.

@esmaeil-ahmadipour esmaeil-ahmadipour force-pushed the feature/define_routes_for_screens branch from fb7f974 to c6c80a3 Compare December 12, 2024 19:07
import 'package:bloc/bloc.dart';
import '../../../src/features/main/language/presentation/bloc/language_bloc.dart';

const languagePrefsKey = 'languagePrefs';
Copy link
Collaborator

Choose a reason for hiding this comment

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

please if dont use remove it


@phoenixit99

import 'package:gui/src/features/welcome/presentation/screen/welcome_page.dart';
import 'route_name.dart';

final GoRouter routerConfig = GoRouter(
Copy link
Collaborator

Choose a reason for hiding this comment

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

all this parts used for register route.

we need another route for basic route.

so create two file one for basic route and another for registration route.
and consume two routes in this file. (in future need move any route parts to self modules.)


@phoenixit99

height: _logoSize,
fit: BoxFit.contain,
),
const SizedBox(height: _spacingBetweenElements),
Copy link
Collaborator

Choose a reason for hiding this comment

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

please add Gap package and use it and replace with SizedBox


@phoenixit99

Comment on lines 14 to 26
String get path => switch (this) {
AppRoute.splash => '/',
AppRoute.dashboard => '/dashboard',
AppRoute.password => '/password',
AppRoute.welcome => '/welcome',
AppRoute.initializeMode => '/initialize-mode',
AppRoute.restorationSeed => '/restoration-seed',
AppRoute.confirmationSeed => '/confirmation-seed',
AppRoute.masterPassword => '/master-password',
AppRoute.validatorConfig => '/validator-config',
AppRoute.initializing => '/initializing',
AppRoute.finish => '/finish',
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove this part and write easy way :)

/// Returns the route path for this enum value.
String get routePath => '/$name';


@phoenixit99

Comment on lines 28 to 40
String get name => switch (this) {
AppRoute.splash => 'splash',
AppRoute.dashboard => 'dashboard',
AppRoute.password => 'password',
AppRoute.welcome => 'welcome',
AppRoute.initializeMode => 'initializeMode',
AppRoute.restorationSeed => 'restorationSeed',
AppRoute.confirmationSeed => 'confirmationSeed',
AppRoute.masterPassword => 'masterPassword',
AppRoute.validatorConfig => 'validatorConfig',
AppRoute.initializing => 'initializing',
AppRoute.finish => 'finish',
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

and for this write :

/// Returns the route name for this enum value.
String get routeName => name;


@phoenixit99

@esmaeil-ahmadipour esmaeil-ahmadipour merged commit 4a1a57b into develop Dec 12, 2024
2 checks passed
@esmaeil-ahmadipour esmaeil-ahmadipour deleted the feature/define_routes_for_screens branch December 12, 2024 19:41
@esmaeil-ahmadipour esmaeil-ahmadipour added review done Review done and ready to merge and removed review failed Review not approved, needs changes. labels Dec 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Add/Create new feature review done Review done and ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Define and organize routes for screens
2 participants