-
Notifications
You must be signed in to change notification settings - Fork 43
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
[UI Enhancements]: Local Storage, Responsive CSS, and Response Regeneration #154
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.
Welcome to Our repository.🎊 Thank you so much for taking the time to point this out.
Concerned Project Admin @rawatji001 , please refer to my PR for issue #146 and if it's possible kindly merge this PR under any Level of GSSOC-extd and HackoctoberFest. Thanks in Anticipation. |
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.
amazing work @AdityaP700, these are really meaningful changes. There is one thing we need to reconsider, we would want a guy to only get one wife, if he regenerates , he should go back to the part where name is entered. what do you say : )
your PR title, description everything is so well done, amazing brother, keep this up while contributing to larger projects too : ) |
Hello @1Shubham7 ,thanks for your appreciation . Well i did had a thought of considering your point but since i was assigned to fix my issue so i simply made the changes accordingly . But since your suggestion seems legit ,i would surely update the codebase soon and raise the PR . Again Thanks for your kind Words :) ::Update : Had some issues persisting yet got clicked for review request ,will do the needful changes soon . |
Hello @1Shubham7 ,Apologies for the delay. I was addressing a few other issues, but everything should be in order now. Please take a look at the latest changes when you have a chance. If you have any feedback, suggestions, or notice any issues, feel free to let me know. Looking forward for your input :) bandicam.2024-10-08.02-12-41-621.mp4 |
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.
hey @AdityaP700, you are a bit late, I merged someone's PR about response regeneration (we call it "Play again") . but this PR is really meaningful, I would request you to remove the response regeneration part from the PR and it is a good PR for local storage and CSS : )
once you are done with changes, please change a video of how the website looks after that : ) |
Hello @1Shubham7 ,
bandicam-2024-10-08-14-28-25-722_jQ58eYzP.mp4 |
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.
please don't remove the playAgain button, make those changes : )
@@ -66,13 +103,6 @@ function ConfirmationCheckbox() { | |||
setPerson(wives[randomNum]); | |||
}; | |||
|
|||
const playAgain = () => { |
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.
why are you removing the playAgain button? please don't remove 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.
Apology for removing it ,will be adding it back with all functionality for sure
Hello @1Shubham7,Apologies for the delays and any confusion caused. As a beginner, this was one of my first issues, and I’ve learned a lot throughout the process—whether it’s using GitHub effectively, implementing React hooks, or performing responsive checks. I’ve made my latest commit, and I would appreciate it if you could kindly review it. |
You have done amazing work brother, keep it up! thanks a lot for your contribution : ) I think can be merged now : ) |
Hey there @1Shubham7 ,Well I have noticed that the PR which have been merged hasn't got updated in the leaderboard yet despite been for 2 weeks ,at first I thought due to some issues it wasn't updated so I kept waiting ,But still the issue has been there despite having the necessary Labels . Well I was suggested by the concerned admins to contact you regarding this so ,kindly look into this so as my PR can be reflected in the Leaderboard. Thanks in anticipation:) |
Overview:
This pull request refers to issue #146 which introduces several enhancements to the project, focusing on user experience and responsiveness.
Changes Made:
bandicam-2024-10-05-19-00-27-750_5SdOuUKG.mp4