Skip to content

Fatimah Almousa -task-2T2#3

Open
FatimahAlMousa wants to merge 1 commit intoGDSC-IAU:masterfrom
FatimahAlMousa:master
Open

Fatimah Almousa -task-2T2#3
FatimahAlMousa wants to merge 1 commit intoGDSC-IAU:masterfrom
FatimahAlMousa:master

Conversation

@FatimahAlMousa
Copy link

task2 four pages home page, search page, notifications page, and profile page.

@arwaalkhathlan arwaalkhathlan self-assigned this Mar 18, 2024
Copy link

@arwaalkhathlan arwaalkhathlan left a comment

Choose a reason for hiding this comment

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

your app was good but here is a few points i would give you.

  1. file organization but having a widget file and an asset file also would make it easier for you.
  2. while your code was clean it had a few issues with how you implemented some stuff i tried to point most of them out hope it helps!!
  3. not very readable try to utilize comments more like you did in your profile page.


final List<Widget> pages = [
HomePage(),
Scaffold(

Choose a reason for hiding this comment

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

you don't need the scaffold it doesn't serve any purpose you can get rid of it and replace it with
SearchPage(),

Suggested change
Scaffold(
SearchPage(),

Comment on lines +33 to +59
if (_currentIndex == 1) {
Navigator.of(context)
.push(MaterialPageRoute(
builder: (context) => SearchPage(),
))
.then((value) {
// This code runs after returning from SearchPage
// You can add any additional logic here
});
} else if (_currentIndex == 2) {
Navigator.of(context)
.push(MaterialPageRoute(
builder: (context) => NotificationsPage(),
))
.then((value) {
// This code runs after returning from NotificationsPage
// You can add any additional logic here
});
} else if (_currentIndex == 3) {
Navigator.of(context)
.push(MaterialPageRoute(
builder: (context) => ProfilePage(),
))
.then((value) {
// This code runs after returning from ProfilePage
// You can add any additional logic here
});

Choose a reason for hiding this comment

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

this whole code is not needed because the set state already does the job this is just extra.

Comment on lines +43 to +44
Row(
mainAxisAlignment: MainAxisAlignment.spaceAround,

Choose a reason for hiding this comment

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

Suggested change
Row(
mainAxisAlignment: MainAxisAlignment.spaceAround,
SizedBox(
height: 200,
child: ListView(
scrollDirection: Axis.horizontal,

replace your Row with sized box with a child listview the scrolls horizontally so it doesn't overflow on the side.

@@ -0,0 +1 @@
// TODO Implement this library. No newline at end of file

Choose a reason for hiding this comment

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

no need for this page

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

class CardRow extends StatelessWidget {

Choose a reason for hiding this comment

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

Suggested change
class CardRow extends StatelessWidget {
class pagesRow extends StatelessWidget {

better if the name of the class has the name of the page.

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

Choose a reason for hiding this comment

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

since this is a widget it should be in a widget file instead.

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

Choose a reason for hiding this comment

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

since this is a widget it should be in a widget file instead.

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