-
Notifications
You must be signed in to change notification settings - Fork 16
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
Add Digital Clock Project. #25
Add Digital Clock Project. #25
Conversation
👋 @DarkSlayer102 |
Suggestion has been added in respect of the following best practice Co-authored-by: Shamith <[email protected]>
I have updated the code as pre requested. Specifically, following the best practices that has been suggested
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.
Hi @DarkSlayer102, I have reviewed you code and it needs some serious changes need to be made. Please go through the following code review and make the respective changes AFTER READING THEM CAREFULLY.
Removing unnecessary comments Co-authored-by: Shamith <[email protected]>
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.
Hi @DarkSlayer102, It seems you have implemented my suggestion within the given comment before code review. I believe there a better approach to that. Please check out the following review comments.
All main suggestions have been included in the code.
Hi @iamwatchdogs, I have implemented the suggestions from your code review and appreciate your feedback. I now understand the reasoning behind them. |
It's great to hear that you have made the required changes @DarkSlayer102, I really appreciated you patience and interest to learn best practice and implementing them as suggested. I gotta admit, you were the one who has followed through the suggestion without having multiple iteration of code review. You really did a great job. We really hope you understand some of the core fundamental of writing better code while following some of the best practices. We hope to see you making more contributions to our repos. Thank you for making your contributions. |
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.
Reviewed and Approved by @iamwatchdogs.
Before we proceed to merging this PR, I might add a last suggestion to you, @DarkSlayer102. You see, unlike other repo, we have a contribution page where all the contribution made by different people are reflected. So, if you write a README.md file within your project folder explain about you project, it will be reflected within the contribution page. You can checkout one of the final result here. Note This is totally optional, and wa can proceed to merging this PR into repo with or without the README.md. But, it could be a great addition to your contribution both in here and on contribution page. |
Hi @DarkSlayer102, I think there's been a mistake and you have closed the PR. So, I have reopened it. Let me know if you want to proceed to merging the PR or wait till you add your README.md file. And Please do let me know if you need any help with it. |
Hello @iamwatchdogs I apologize for closing the PR earlier. I have now added the README.md file, and I believe the PR is ready to be merged. Thank you for your patience. |
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.
Reviewed and Approved by @iamwatchdogs.
Thank you for contributing @DarkSlayer102. Make sure to check your contribution on GitHub Pages. |
No description provided.