issue 12 solved #17
issue 12 solved #17
Conversation
There was a problem hiding this comment.
One general comment: It looks like your text editor removed all the TODO: text from the TODO comments... could you turn that setting off and replace the TODO:s? We use those as markers that are easy to search for when we're looking for the small tasks that have emerged in the code.
I've made a few other comments on the files you've submitted. Your changes are looking pretty good! There are just a few things to double-check or correct and then we should be able to accept this PR.
Oh! One last note: You'll need to "sign" (digitally is fine) the Contributor Agreement:
https://docs.google.com/document/d/14GymHIfYAWZy3kIqEIvqyIDPoi6lNwE0AQ8rdd9iuUk/edit
...you can just make a copy of that file, add your changes to the bottom, and send me the updated file or print it to PDF and send me that. Thanks!
|
In pubspec.lock
<#17 (comment)>:
@@ -218,6 +225,13 @@ packages:
url: "https://pub.dartlang.org"
source: hosted
version: "1.1.1"
+ fake_async:
I'm not sure what this new dependency (fake_async) is. Is it necessary?
------------------------------
In pubspec.lock
<#17 (comment)>:
@@ -113,13 +113,20 @@ packages:
url: "https://pub.dartlang.org"
source: hosted
version: "2.1.0+1"
+ characters:
I'm not sure what this new dependency (characters) is. Is it necessary?
Even I don't know about this new changes as it's coming after I updated my
Flutter to the latest version. But I'll try an research and let you know
about these changes as soon as I learn about it.
And for the rest yes I will make the required changes and commit again.
…On Thu, 13 Aug 2020 at 23:34, Steven Deobald ***@***.***> wrote:
***@***.**** requested changes on this pull request.
One general comment: It looks like your text editor removed all the TODO:
text from the TODO comments... could you turn that setting off and replace
the TODO:s? We use those as markers that are easy to search for when
we're looking for the small tasks that have emerged in the code.
I've made a few other comments on the files you've submitted. Your changes
are looking pretty good! There are just a few things to double-check or
correct and then we should be able to accept this PR.
Oh! One last note: You'll need to "sign" (digitally is fine) the
Contributor Agreement:
https://docs.google.com/document/d/14GymHIfYAWZy3kIqEIvqyIDPoi6lNwE0AQ8rdd9iuUk/edit
...you can just make a copy of that file, add your changes to the bottom,
and send me the updated file or print it to PDF and send me that. Thanks!
------------------------------
In pubspec.lock
<#17 (comment)>:
> @@ -218,6 +225,13 @@ packages:
url: "https://pub.dartlang.org"
source: hosted
version: "1.1.1"
+ fake_async:
I'm not sure what this new dependency (fake_async) is. Is it necessary?
------------------------------
In pubspec.lock
<#17 (comment)>:
> @@ -113,13 +113,20 @@ packages:
url: "https://pub.dartlang.org"
source: hosted
version: "2.1.0+1"
+ characters:
I'm not sure what this new dependency (characters) is. Is it necessary?
------------------------------
In .vscode/launch.json
<#17 (comment)>:
> @@ -0,0 +1,14 @@
+{
+ // Use IntelliSense to learn about possible attributes.
+ // Hover to view descriptions of existing attributes.
+ // For more information, visit: https://go.microsoft.com/fwlink/?linkid=830387
+ "version": "0.2.0",
+ "configurations": [
+ {
+ "name": "Flutter",
+ "program": "lib/main_sand.dart",
+ "request": "launch",
+ "type": "dart"
+ }
+ ]
+}
I'm guessing this is necessary for most people who are editing the project
with VS Code? I think it might be a better option to git-ignore the
/.vscode directory instead, but this is probably worth a discussion in
#hackathon-flutter-pariyatti with one of the mentors who uses VS Code on a
daily basis. :)
------------------------------
In lib/ui/screens/HomeScreen.dart
<#17 (comment)>:
> @@ -51,11 +51,17 @@ class _HomeScreenState extends State<HomeScreen> {
Icons.today,
color: Color(0xff6d695f),
),
+ activeIcon: Icon(
+ Icons.today,
+ color: Colors.brown,
It might seem kind of finicky, but the colour in the wireframes isn't
exactly brown. If you use an eyedropper tool in an image editing app you'll
see the colour is RGB = 186, 86, 38 or a hex code of #ba5626. We should
probably use the exact colour the designers had chosen.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#17 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ANVNFNQ5O5NJJAML2L2Q6F3SAQTLPANCNFSM4P6VN4RQ>
.
|
|
@jHetvi @deobald don't bother with changes in @deobald ideally |
config/app_config.sample.json
Outdated
| @@ -1,3 +0,0 @@ | |||
| { | |||
There was a problem hiding this comment.
@jHetvi we have to copy this file to config/app_config.json and not move it there. We will need to keep the config/app_config.sample.json file available for people who work on this in the future.
Can you please add this file back?
There was a problem hiding this comment.
Hi @jHetvi I think the PR is looking much better now! @VarunBarad has made a few comments: We'll ignore pubspec.lock after your PR is merged and we'll need you to put config/app_config.sample.json back.
I noticed one last thing I should have seen from the beginning... It looks like this PR is between your master branch and the pariyatti/patta master branch. Unfortunately, we'll have to receive the PR into the pariyatti/patta development branch. Could you change your PR to point to the development branch upstream or recreate it? Thanks!
|
Closing this PR because it's accidentally against the |
No description provided.