Closed IngridFuentes closed 3 years ago
@IngridFuentes this is a great start, kudos 👏 ❤️ . I have left some comments, try to work around them, it will enhance your CSS knowledge. If you have questions, please don't hesitate to ask here or on slack, best of luck :)
@princiya Thank you so much for your feedback! I'm on it right now.
@IngridFuentes I noticed you said you resolved some of the comments brought up, but I am not seeing the changes. Do you think you may not have pushed your changes?
@IngridFuentes I noticed you said you resolved some of the comments brought up, but I am not seeing the changes. Do you think you may not have pushed your changes?
I haven't pushed the changes yet. I'm still fixing the first issue above. Sorry for the confusion @sunithapatel
@IngridFuentes I noticed you said you resolved some of the comments brought up, but I am not seeing the changes. Do you think you may not have pushed your changes?
I haven't pushed the changes yet. I'm still fixing the first issue above. Sorry for the confusion @sunithapatel
No problem at all, take your time!
@IngridFuentes thanks for working on the feedback suggestions. Let us know when the PR is ready, and we can merge. Like Sunitha mentioned, take your time and if you need help let us know :)
@princiya @sunithapatel I finished working on the issues. I'd like you to take a look. I pushed the changes. What do I need to do next? Thanks
Nice job @IngridFuentes! I left two review comments - one about taking out commented out code and a now unused component. Could you address that now?
Another about potentially not handling rendering on a smaller screen - but we could resolve that in another issue - do you agree @princiya?
Other than that, things look good to me! Nicely done! :)
Thank you @sunithapatel Let me know if you guys will make a new issue for that. Otherwise, I'll work on that and learn too :) Thank you!
Looks like there is a merge conflict with deleting that file. You may need to update your fork with the latest changes.
@sunithapatel I updated the fork. Can you please take a look and tell me everything is okay? Thank you! :)
Yeah it looks great!!! Good job! 👏🏼
Sorry - one last thing! There was a test that verified the text in the Main.js component and now it fails so we won’t be able to merge this.
Sorry I didn’t realize it before. You can delete the test for now to get the build fixed and we can merge this. Can create separate issue to add tests.
@sunithapatel I just want to verify I don't delete the wrong file. The file you are talking about is the App.test.js, right?
Yes, that’s correct, that is the file to remove.
Once you do that before you push the change can you run npm test
locally to make sure it won’t error? This is what the build is doing here so just want to make sure.
Sorry for all the extra work!
@sunithapatel No worries at all! I'm happy to do it. I just did that and no error. It just says "No tests found"
Cool, thank you!!! I was thinking it would do that. Go ahead and push the changes.
@sunithapatel Great! I pushed the changes.
@IngridFuentes I am really sorry about this, but it looks like since we have a step to run tests, it wants at least one test.
So can you put back the App.test.js file and change the file content to this instead?
import { render, screen } from '@testing-library/react';
import App from './App';
test('renders title on Home page', () => {
render(<App />);
const titleElement = screen.getByText('Open Source Internships');
expect(titleElement).toBeInTheDocument();
});
Hopefully that should be it!
@sunithapatel Sure!
@sunithapatel Test file recovered! Let me know if that works. Thank you :)
Yay!!!! Great job @IngridFuentes! 👏🏼🚀
Great job @IngridFuentes and @sunithapatel for the moderation here. :)
@sunithapatel Awesome! Thank you, guys! @sunithapatel @princiya
I designed a Home page that gives a small description of what this site is for. It is a site for Open Source Internships.
"Fixes #10 "