Skip to content

Faris Alzayed - Task 3 Done.#4

Open
FarisAI24 wants to merge 2 commits intoProgramming-Club-IAU:masterfrom
FarisAI24:master
Open

Faris Alzayed - Task 3 Done.#4
FarisAI24 wants to merge 2 commits intoProgramming-Club-IAU:masterfrom
FarisAI24:master

Conversation

@FarisAI24
Copy link

No description provided.

Copy link

@Radwan-Albahrani Radwan-Albahrani left a comment

Choose a reason for hiding this comment

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

Loving the dark mode design. Excellent work. Below are my recommendations to improve code modularity, and one recommendation that should improve performance and help later on when API is implemented.

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

class WeatherPredModel {

Choose a reason for hiding this comment

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

Excellent use of the model.

});
}

List<WeatherPredModel> weatherPredList = [

Choose a reason for hiding this comment

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

Good way to add dummy data. Very useful

);
}

Expanded _weatherDays() {

Choose a reason for hiding this comment

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

For this function. Instead of making 3 columns for the name, icon, and degree, You can make each weathermodel data a row, and the row contains all this info. That way you will have to use List.generate only once, improving performance.

return const Row(
mainAxisAlignment: MainAxisAlignment.spaceEvenly,
children: [
Column(

Choose a reason for hiding this comment

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

Every column in this row is repeated. Two recommendations.

  1. Make that column a modular widget so its easy to reuse it for all the columns you'll make
  2. Make the row scrollable so you can add more than 4 times. This will be helpful when we start using the API

);
}

Column weatherStatus() {

Choose a reason for hiding this comment

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

You can make this widget modular and have it take in all the text as input. This way you'll be able to change it based on the user's location

);
}

AppBar _appBar() {

Choose a reason for hiding this comment

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

This is excellent. Removes the app bar clutter from the main stateless widget. Excellent work!

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