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

fixed issue #4 #8

Merged
merged 3 commits into from
Oct 31, 2024
Merged

fixed issue #4 #8

merged 3 commits into from
Oct 31, 2024

Conversation

RITIK-CHAUDHRY
Copy link

@RITIK-CHAUDHRY RITIK-CHAUDHRY commented Oct 17, 2024

Description:

Implement the 2 Priority Tasks for the Day

Related Issue:

fixes issue #4

Type of change:

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update
Motivation and Context
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

Screenshots (if appropriate):

Screen_Recording_20241026_084748_Easy.2.Do.mp4

Checklist:

I have registered myself at Contrihub website.
My code follows the code style of this project.
I have commented my code, particularly in hard-to-understand areas
My changes generate no new warnings
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
Any dependent changes have been merged and published in downstream modules
Test Configuration:

Firmware version:
Hardware:
Toolchain:
SDK:

@mukul-dev
Copy link
Collaborator

@RITIK-CHAUDHRY Quite a few things need to be corrected like:

  1. The motivational_message resource is I guess missing that you can fix easily.

  2. When user mark a Task as completed, they do not need to click the save tasks button after that... So, once they click checkbox you can use some alert to confirm whether to mark it or not (it cannot be changed once marked, that's working correct as of now). This is required because when a user marks and doesn't click Save and leave .. after some time, the app might be removed from running in the background.. So, it should be saved after confirming...

  3. The UI could be better its clean and minimal as of now but try giving it some custom style (the input field).. right now in some of my devices it fully covers the width and seems like its getting somewhere out of the screen so provide margin to them.

  4. The motivational_message is supposed to be a random quote regarding task completion to make user cheer up.. Also you can simply use a toast message instead of a text field! For now if one task is done , marked and saved. The message displays "Great, you have your first priority task" but if after that the app is restarted it gets removed and default text appears.

  5. If user marks second task before marking the first one, it displays "Awesome! You've completed both priority tasks!", but it's not the case!

  6. I didn't check this but do consider empty these input field when the day changes (at midnight 00:00 am) if not yet implemented i.e. to reset for the next day.

Do more of testing with different situations and activities within this feature considering yourself using the app, it will make you find bugs easily!
You've done the work nicely just a bit of more correction is all it needs!

@RITIK-CHAUDHRY
Copy link
Author

@mukul-dev sir i am working on the improvements that you had mentioned just after your comment. I have submitted the pr link in contrihub. now its showing only 50 min left. so the changes that i done in this will give me points if i will submit the changes mentioned by you after 50min or i have to constraint myself to 50min

RITIK-CHAUDHRY added a commit to RITIK-CHAUDHRY/Easy2Do that referenced this pull request Oct 18, 2024
@mukul-dev
Copy link
Collaborator

@RITIK-CHAUDHRY No, you can work and fix ... your PR request is still there, and I will provide points and feedback accordingly by tomorrow after reviewing so you can let me know when you are done with fixing them...

@RITIK-CHAUDHRY
Copy link
Author

@mukul-dev i made changes from my side please have a look on it.

@mukul-dev
Copy link
Collaborator

@mukul-dev i made changes from my side please have a look on it.

There are a few errors still in the code when i pulled this version.. can you please share the screenshots of working app (for this fragment only) ...

@RITIK-CHAUDHRY
Copy link
Author

RITIK-CHAUDHRY commented Oct 19, 2024

@mukul-dev sir can you elaborate the errors that you have encountered so that i can fix those and get back to you

@mukul-dev
Copy link
Collaborator

mukul-dev commented Oct 19, 2024

@mukul-dev sir can you elaborate the errors that you have encountered so that i can fix those and get back to you

Yes @RITIK-CHAUDHRY , actually some of them are not really that big like just identifier/token name is wrong that I can fix while testing.. Actually, I am sharing the screenshot of IDE after pulling your request where there are errors of missing files and some code errors.

IMAGE 1:
Screenshot 2024-10-19 234039

IMAGE 2:
image

At first I thought there is some issue while pulling but I checked in commits here on GitHub and in your last commit everything is just like this... so there might be something faulty when you committed changes... If the app is working on your IDE with your code you can share that too I will try to check with that... because the implementation seems correct but just these few errors are there..

@mukul-dev
Copy link
Collaborator

@RITIK-CHAUDHRY you can commit changes (if you rectified these errors) by 10 pm today, because there are more people who want to work on this issue. So, I have to assign it to them!

@RITIK-CHAUDHRY
Copy link
Author

RITIK-CHAUDHRY commented Oct 20, 2024

yes sir i am working into the issues right now. it takes time as i am trying that no further issues will be raised. i will try to commit changes as soon as possible. @mukul-dev

@mukul-dev
Copy link
Collaborator

@RITIK-CHAUDHRY sure .. try finishing before 10pm so that i can review and accept.. or assign others!

@RITIK-CHAUDHRY
Copy link
Author

@mukul-dev sir can you help me on this
image
do i add this file to provide a custom background for the input fields in the fragment_2priority_task_rule.xml layout file.

@mukul-dev
Copy link
Collaborator

@RITIK-CHAUDHRY yes you can add this for customising the style.. just assign this to style or background attribute of the input fields in the xml

@RITIK-CHAUDHRY
Copy link
Author

@mukul-dev sir review it

@mukul-dev
Copy link
Collaborator

@RITIK-CHAUDHRY There are still a lot of things that aren't working... and some were working before but not working now... like why you removed the code for SAVE button... now its not saving the tasks.. rest things can not be tested without saving .. The Resetting the input box at 00:00 am is also removed.. and so on... I cannot merge this as of now ... You have to implement and try running on your mobile phone so that you get to know its working as expected or not... As of now I have to assign it to the other guy but if you are still interested in working for this feature you can later submit this..
Meanwhile you can work for #3 which you requested for! All the best for that!

@RITIK-CHAUDHRY
Copy link
Author

RITIK-CHAUDHRY commented Oct 21, 2024

sorry to hear that sir.i will generate a commit by EOD to mitigate all problems @mukul-dev

@RITIK-CHAUDHRY
Copy link
Author

@mukul-dev sir i have rasised PR please review last time. please read the description.

@RITIK-CHAUDHRY
Copy link
Author

@mukul-dev can you please update me on the last PR

fixed issue ContriHUB#4 . in this version of the app the user can give input of the tasks and henceforth can mark as done when it is done. a pop up will come which ensure that use confirm the marking. task2 can be marked as done only if task 1 is completed
@RITIK-CHAUDHRY
Copy link
Author

@mukul-dev sir i have modified my PR also i have done all the changes that are previously mentioned. sorry for the inconvinience caused due to delays.

@RITIK-CHAUDHRY
Copy link
Author

@mukul-dev sir please review this PR too and if its okay please merge it

@mukul-dev
Copy link
Collaborator

@RITIK-CHAUDHRY the text being dispayed in the text box doesnt match the state actually... below are some screenshots for you reference..
Screenshot_20241028_230335_Easy 2 Do
Screenshot_20241028_230405_Easy 2 Do
Screenshot_20241028_230442_Easy 2 Do

also user is allowed to complete any task first (not just the first one, like implemented now)...
Screenshot_20241028_230641_Easy 2 Do

I think these are easier to fix.. so you can submit the corrected version and we can finish this!! Great work!

@RITIK-CHAUDHRY
Copy link
Author

@mukul-dev pardon sir for this.. is there anything that i can do extra for bonus that you like to suggest

@mukul-dev
Copy link
Collaborator

@mukul-dev pardon sir for this.. is there anything that i can do extra for bonus that you like to suggest

nothing much as of now... just rectify these bugs.. you may try to improve the UI .. probably with the use of some relevant icons in the fragment if you want!

@RITIK-CHAUDHRY
Copy link
Author

RITIK-CHAUDHRY commented Oct 31, 2024

@mukul-dev sir please review my Pr... today is last day please

@RITIK-CHAUDHRY
Copy link
Author

@mukul-dev sir i raised PR on 29th please review it...today is last date of contrihub and hacktoberfest. If not done today i will lose minimum 4 PR bar.. please merge this request. I made all the changes please sir.

@mukul-dev
Copy link
Collaborator

mukul-dev commented Oct 31, 2024

Sure @RITIK-CHAUDHRY , I tested couple of times this feature since yesterday and you've done a great job! feature is working almost fine. Why I am saying 'almost', is because there is no validation for empty input field of task (User can mark without writing any tasks which should not be the case). Also, when you restart app, the messages disappear which should remain like with the help of it, user gets to know his/her both tasks are done! Like these there are some small issues.
But yes, these are minor things which I wanted to tell you and It's easy for you to fix because you've already done the main task so we can merge this and later these can be fixed! Thanks for your contribution!

@RITIK-CHAUDHRY
Copy link
Author

@mukul-dev thank you so much sir but it is showing approved not merged.

@RITIK-CHAUDHRY
Copy link
Author

@mukul-dev sir please look into this neither on contrihub nor on hacktoberfest it is showing merged

@mukul-dev mukul-dev merged commit 982eba7 into ContriHUB:main Oct 31, 2024
@mukul-dev
Copy link
Collaborator

@RITIK-CHAUDHRY yes.. fixed that! 👍

@RITIK-CHAUDHRY
Copy link
Author

@mukul-dev thanks sir...
Happy diwali

@mukul-dev
Copy link
Collaborator

@mukul-dev thanks sir... Happy diwali

Cool! And a very Happy Diwali to you too !

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