-
Notifications
You must be signed in to change notification settings - Fork 4
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
Added a Home Screen and the ability to add tasks #9
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't try compiling and running this - I'm making the assumption that you wouldn't push it if it didn't compile.
lib/models/task.dart
Outdated
final String title; | ||
Icon taskIcon = Icon(Icons.alarm); | ||
|
||
TaskList({this.title, this.taskIcon}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If title
is empty, this will fail. Mark title
as required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point
lib/models/task_home_data.dart
Outdated
class TaskListHome extends ChangeNotifier { | ||
List<TaskList> taskList = [ | ||
TaskList(title: "Rift's Corner", taskIcon: Icon(Icons.alarm)), | ||
TaskList(title: "Han's Corner", taskIcon: Icon(Icons.run_circle)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Han's => Hans'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ahh I see
} | ||
|
||
void addTask(String myTaskTitle) { | ||
final task = TaskList(title: myTaskTitle); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will you add the taskIcon
later?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thinnk that is something that could be explored in the future. Allowing the user to select a task icon would be cool
lib/screens/add_task_screen.dart
Outdated
Navigator.pop(context); | ||
}, | ||
style: TextButton.styleFrom(backgroundColor: Colors.lightBlue), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Perhaps the color should be specified in a global theme? Then you can use Theme.of(context).xxxx
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mmhhnnn you smart
lib/screens/home.dart
Outdated
style: TextStyle( | ||
color: Colors.white, | ||
fontSize: | ||
MediaQuery.of(context).size.width / 20, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like a dangerous calculation. Maybe set a minimum, and use a Wrap()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Set it to a minimum? what do you mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now, the screen size always determines the font size. If someone happens to have a very small screen, the text could be size 2. Just do a simple if statement. If the resulting font size is smaller than ~10, use a font size of 10. Otherwise, make sure you are using a whole number.
lib/screens/home.dart
Outdated
child: Text('Add A Task', | ||
style: TextStyle( | ||
color: Colors.white, | ||
fontSize: MediaQuery.of(context).size.width / 20, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
lib/screens/home.dart
Outdated
cursorColor: Colors.white, | ||
autocorrect: true, | ||
onChanged: (newTitle) { | ||
print(newTitle); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't forget to remove debugging statements
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
totally forgot
lib/screens/home.dart
Outdated
elevation: 5.0, | ||
padding: EdgeInsets.all(20.0)), | ||
onPressed: () { | ||
Provider.of<TaskListHome>(context, listen: false).addTask(title); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this fails for some reason, we'll still pop. Might want to introduce some additional error handling, especially if this ends up pushing to a network resource (which may be unavailable)
I addressed all the issues you highlighted and added two images to the Readme to show a little progress. Issues touched: Error handling, using ThemeData, debugging statements removed, etc. Currently don't see the need to add a wrap. We could possibly add it or review in the future. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tiny changes, but otherwise looks good.
lib/screens/home.dart
Outdated
], | ||
), | ||
); | ||
backgroundColor: Colors.purple, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe pull from the theme?
lib/screens/home.dart
Outdated
title = newTitle; | ||
}, | ||
textAlign: TextAlign.center, | ||
style: TextStyle(color: Colors.black, fontSize: 20.0), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Theme?
lib/screens/home.dart
Outdated
style: TextStyle(color: Colors.black, fontSize: 20.0), | ||
decoration: InputDecoration( | ||
filled: true, | ||
fillColor: Colors.white, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Theme?....you get the idea....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lol Really not seeing the point of changing every color to use theme. Enlighten me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Light mode vs Dark mode
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. Adding a dark mode later would be cool
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, it keeps things consistent. If all the text should be the same color, using a shared variable saves time and effort when you want to change it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I used Theme
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So close! You can do it, Jason! 📣
lib/screens/home.dart
Outdated
"Create", | ||
style: TextStyle(color: Theme.of(context).canvasColor), | ||
), | ||
backgroundColor: Colors.lightBlueAccent, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Theme 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HOW did I miss these!!! LOL
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
n00b
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lol..... takes one to know one.....
lib/screens/home.dart
Outdated
clipBehavior: Clip.none, | ||
children: <Widget>[ | ||
TextFormField( | ||
cursorColor: Colors.black, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔
lib/screens/home.dart
Outdated
? IconButton( | ||
icon: Icon( | ||
Icons.error, | ||
color: Colors.red, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... I'll accept it
lib/screens/home.dart
Outdated
width: 270, | ||
height: 50, | ||
decoration: BoxDecoration( | ||
color: Colors.white, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔
lib/screens/home.dart
Outdated
), | ||
ElevatedButton( | ||
style: ElevatedButton.styleFrom( | ||
primary: Colors.lightBlue, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔
lib/screens/home.dart
Outdated
} | ||
}, | ||
child: Text('Add Task', | ||
style: TextStyle(color: Colors.white, fontSize: 20.0)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔
This time for sure! None escaped my sight |
Why not merge the pull request |
Because I don't have permissions on this repo 🤷🏻♂️ |
No description provided.