-
-
Notifications
You must be signed in to change notification settings - Fork 799
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
New play - Keeper App #74
Conversation
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/atapas/react-play/3foi64pu7SEBFjtdF6u4wpEoH5cT [Deployment for 7bf26f0 failed] |
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.
First of all, this app idea is great. Keeper gonna be a helpful one to many to learn things from it.
Here are a few comments to take care:
- The style of Header and Footer is conflicting the style of platform. So please scope your styling to your play. The side effect has been pointed by @6km in a comment already: New play - Keeper App #74 (comment)
- As ReactPlay already has a footer, you can avoid adding another footer with date and copyright. So, IMO that component can be avoided.
- I saw you included, Material. The Community is debating on what is the Style system we should go ahead with, Material, Bootstrap, Tailwind: Using Tailwind CSS as a CSS Framework #73. Until the decision is made, the reactplay platform is going to use raw CSS. So at this stage we will not include material package.
- I have seen, you are using the icons from material, we are already packaged react-icons. Please search your equivalent icon in react-icons: https://react-icons.github.io/react-icons/ and replace then with material. So basically, no material. If you need additional icons, ask me. We will provide. If the app looks a bit different without material, it is fine. If you need styling help, please reach out to me.
- Do not commit the package.lock file, yarn.lock enough
- What is the thumbnail.png for? We are not using it. If it is for cover image, the name should be cover.png
- Please drop a Readme.md file at the root of your play folder to explain what is he play about what it uses, etc. Here is an example: https://github.com/atapas/react-play/blob/main/src/plays/states/Readme.md
- I have given other code level comments too.
At last, Please take your time to address the review comments. No hurry. We are here to assist you on anything needed to get it merged 😀 and I'm just a "Google meet/Zoom" away.
Happy Coding, Happy Learning!
@atapas Thanks for pointing out these issues, I will remove that all, to make it all good. |
Hello @Shivam-Katare We have upgraded to React v18 as part of this issue: #87 We have tested the existing plays. All good. Please pull this change to your branch and keep it up-to-date. You have to do sanity of things to make sure everything works for your changes in your branch. Happy Coding! |
@Shivam-Katare Once you done, please remove the work-in-progress label and review-ready label so that we can start the review. |
@Shivam-Katare Build is successful but 3 comments are pending. |
@atapas all conversations resolved, you can check it now. |
@Shivam-Katare All good.. Do you want to make the cover image a bit better? It looks steatched. Maybe do not make a transparent image, give it a background using Canva? Let me know if you need help? |
@atapas Cover image changed. |
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.
Bro, again package-lock.json file is back in the commit. Please remove that and commit.
@Shivam-Katare This is good to merge. Tested. You just need to take care of removing package lock json file from commit. |
@atapas Shall I put "package-lock.json" in git ignore? |
It is already in .gitignore |
You can do git rm and commit, push.. Btw, I have captured this case in the Merge Conflict video I published today 😂 |
@atapas Yes, I have seen the video 😀and, removed package lock.json. You can now check 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.
@Shivam-Katare looks good.
@all-contributors please add @Shivam-Katare for Code |
I've put up a pull request to add @Shivam-Katare! 🎉 |
I am trying.
…On Tue, May 3, 2022, 09:19 Tapas Adhikary ***@***.***> wrote:
Hey, @atapas <https://github.com/atapas> all conversations are resolved.
Please resolve merge conflicts.
—
Reply to this email directly, view it on GitHub
<#74 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AV3VDYI3KZIOIJR2MARPNIDVICO43ANCNFSM5T3UTNAQ>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Description
I have added the new play named Keeper. This is the clone of Google Keep where we can add and delete our notes. This "Keeper app" save your notes until you delete them.
How Has This Been Tested?
Install some martial UI dependencies for testing styling -
emotion/react
emotion/styled
material-ui/core
material-ui/icons
mui/icons-material
mui/material
Start the app by npm start or yarn start to see it.
Checklist:
App Preview(ScreenShots)