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

Created navigation bar #30

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Created navigation bar #30

wants to merge 2 commits into from

Conversation

wdan31
Copy link

@wdan31 wdan31 commented Oct 13, 2024

With placeholders for camera, logs, and SITL pages.

@wdan31 wdan31 closed this Oct 13, 2024
@wdan31 wdan31 reopened this Oct 13, 2024
Copy link
Contributor

@BalajiLeninrajan BalajiLeninrajan left a comment

Choose a reason for hiding this comment

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

So far so good! Left a couple of comments and you need to add unit tests, msg on your thread if you need help or clarification.

@@ -0,0 +1,64 @@
import 'package:flutter/material.dart';

class NavBar extends StatefulWidget {
Copy link
Contributor

Choose a reason for hiding this comment

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

Add some documentation in the form of comments, check the other widgets for examples

@@ -17,6 +18,9 @@ class App extends StatelessWidget {
),
routes: {
'/': (BuildContext context) => HomePage(title: 'WARG IMACS'),
'/logs': (BuildContext context) => const Placeholder(child: NavBar()),
Copy link
Contributor

Choose a reason for hiding this comment

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

Not exactly a requirement, but you can make the placeholders look a lot nicer

class PlaceholderScreen extends StatelessWidget {
  const PlaceholderScreen({Key? key, required this.title}) : super(key: key);

  final String title;
  @override
  Widget build(BuildContext context) {
    return Scaffold(
        appBar: AppBar(
          title: Text(title),
        ),
        body: const Placeholder(),
        bottomNavigationBar: const NavBar());
  }
}

}

class NavBarState extends State<NavBar> {
int _index = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

If your going with this implementation, I would actually recommend getting rid of the router and replacing it with a list or map of screen widgets. You'd essentially be replacing the router and making a single screen app with interchangeable screens. Since we are using a router there is some weirdness with the index.


switch (index) {
case 0:
Navigator.of(context).popUntil((route) => route.isFirst);
Copy link
Contributor

Choose a reason for hiding this comment

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

Not really a fan of routing like this, use pushReplacementNamed and popAndPushNamed when possible or replace the router as suggested above.

Navigator.of(context).popUntil((route) => route.isFirst);
break;
case 1:
Navigator.pushNamed(context, '/logs');
Copy link
Contributor

Choose a reason for hiding this comment

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

Use pushReplacementNamed or popAndPushNamed or replace the router as suggested above.

Navigator.pushNamed(context, '/logs');
break;
case 2:
Navigator.pushNamed(context, '/camera');
Copy link
Contributor

Choose a reason for hiding this comment

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

Use pushReplacementNamed or popAndPushNamed or replace the router as suggested above.

Navigator.pushNamed(context, '/camera');
break;
default:
Navigator.pushNamed(context, '/sitl');
Copy link
Contributor

Choose a reason for hiding this comment

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

Use pushReplacementNamed or popAndPushNamed or replace the router as suggested above.

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