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.

Sign up for GitHub

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

Jump to bottom

Sprint1 login screen #2

Merged
merged 20 commits into from
Jan 23, 2021
Merged

Sprint1 login screen #2

merged 20 commits into from
Jan 23, 2021

Conversation

shylton
Copy link
Contributor

@shylton shylton commented Jan 20, 2021

Created three views: Login/Register, About (blank), User view (copied Andre's and reformatted)

  • Didn't spend a ton of time with design, more work to be done
  • I am using a HashRouter from 'react-router-dom' and loading components/views into the Main page. So it looks like SPA. One thing I noticed is that it puts information in the browser bar (similar to a GET request). We'll need to modify this behavior, there should be a setting...

AndreEPaul
AndreEPaul reviewed Jan 20, 2021
View reviewed changes
front-end/public/index.html
@@ -2,14 +2,15 @@
<html lang="en">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also wouldn't change the index.html, this is just automatically made by create-react-app and we won't render it anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As it is, index.html is indeed being rendered. That's where the page title and favicon goes. I changed so it wouldn't say React stuff and use React's logo. Don't know how to build a React app without rendering index.html but if that's the case we can remove this file when we get there.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I meant like visually you're only seeing what App.js is rendering. I don't think we can remove the file but I just never really see it customized is all.

AndreEPaul
AndreEPaul reviewed Jan 20, 2021
View reviewed changes
front-end/src/logo.svg
@@ -1 +0,0 @@
<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 841.9 595.3"><g fill="#61DAFB"><path d="M666.3 296.5c0-32.5-40.7-63.3-103.1-82.4 14.4-63.6 8-114.2-20.2-130.4-6.5-3.8-14.1-5.6-22.4-5.6v22.3c4.6 0 8.3.9 11.4 2.6 13.6 7.8 19.5 37.5 14.9 75.7-1.1 9.4-2.9 19.3-5.1 29.4-19.6-4.8-41-8.5-63.5-10.9-13.5-18.5-27.5-35.3-41.6-50 32.6-30.3 63.2-46.9 84-46.9V78c-27.5 0-63.5 19.6-99.9 53.6-36.4-33.8-72.4-53.2-99.9-53.2v22.3c20.7 0 51.4 16.5 84 46.6-14 14.7-28 31.4-41.3 49.9-22.6 2.4-44 6.1-63.6 11-2.3-10-4-19.7-5.2-29-4.7-38.2 1.1-67.9 14.6-75.8 3-1.8 6.9-2.6 11.5-2.6V78.5c-8.4 0-16 1.8-22.6 5.6-28.1 16.2-34.4 66.7-19.9 130.1-62.2 19.2-102.7 49.9-102.7 82.3 0 32.5 40.7 63.3 103.1 82.4-14.4 63.6-8 114.2 20.2 130.4 6.5 3.8 14.1 5.6 22.5 5.6 27.5 0 63.5-19.6 99.9-53.6 36.4 33.8 72.4 53.2 99.9 53.2 8.4 0 16-1.8 22.6-5.6 28.1-16.2 34.4-66.7 19.9-130.1 62-19.1 102.5-49.9 102.5-82.3zm-130.2-66.7c-3.7 12.9-8.3 26.2-13.5 39.5-4.1-8-8.4-16-13.1-24-4.6-8-9.5-15.8-14.4-23.4 14.2 2.1 27.9 4.7 41 7.9zm-45.8 106.5c-7.8 13.5-15.8 26.3-24.1 38.2-14.9 1.3-30 2-45.2 2-15.1 0-30.2-.7-45-1.9-8.3-11.9-16.4-24.6-24.2-38-7.6-13.1-14.5-26.4-20.8-39.8 6.2-13.4 13.2-26.8 20.7-39.9 7.8-13.5 15.8-26.3 24.1-38.2 14.9-1.3 30-2 45.2-2 15.1 0 30.2.7 45 1.9 8.3 11.9 16.4 24.6 24.2 38 7.6 13.1 14.5 26.4 20.8 39.8-6.3 13.4-13.2 26.8-20.7 39.9zm32.3-13c5.4 13.4 10 26.8 13.8 39.8-13.1 3.2-26.9 5.9-41.2 8 4.9-7.7 9.8-15.6 14.4-23.7 4.6-8 8.9-16.1 13-24.1zM421.2 430c-9.3-9.6-18.6-20.3-27.8-32 9 .4 18.2.7 27.5.7 9.4 0 18.7-.2 27.8-.7-9 11.7-18.3 22.4-27.5 32zm-74.4-58.9c-14.2-2.1-27.9-4.7-41-7.9 3.7-12.9 8.3-26.2 13.5-39.5 4.1 8 8.4 16 13.1 24 4.7 8 9.5 15.8 14.4 23.4zM420.7 163c9.3 9.6 18.6 20.3 27.8 32-9-.4-18.2-.7-27.5-.7-9.4 0-18.7.2-27.8.7 9-11.7 18.3-22.4 27.5-32zm-74 58.9c-4.9 7.7-9.8 15.6-14.4 23.7-4.6 8-8.9 16-13 24-5.4-13.4-10-26.8-13.8-39.8 13.1-3.1 26.9-5.8 41.2-7.9zm-90.5 125.2c-35.4-15.1-58.3-34.9-58.3-50.6 0-15.7 22.9-35.6 58.3-50.6 8.6-3.7 18-7 27.7-10.1 5.7 19.6 13.2 40 22.5 60.9-9.2 20.8-16.6 41.1-22.2 60.6-9.9-3.1-19.3-6.5-28-10.2zM310 490c-13.6-7.8-19.5-37.5-14.9-75.7 1.1-9.4 2.9-19.3 5.1-29.4 19.6 4.8 41 8.5 63.5 10.9 13.5 18.5 27.5 35.3 41.6 50-32.6 30.3-63.2 46.9-84 46.9-4.5-.1-8.3-1-11.3-2.7zm237.2-76.2c4.7 38.2-1.1 67.9-14.6 75.8-3 1.8-6.9 2.6-11.5 2.6-20.7 0-51.4-16.5-84-46.6 14-14.7 28-31.4 41.3-49.9 22.6-2.4 44-6.1 63.6-11 2.3 10.1 4.1 19.8 5.2 29.1zm38.5-66.7c-8.6 3.7-18 7-27.7 10.1-5.7-19.6-13.2-40-22.5-60.9 9.2-20.8 16.6-41.1 22.2-60.6 9.9 3.1 19.3 6.5 28.1 10.2 35.4 15.1 58.3 34.9 58.3 50.6-.1 15.7-23 35.6-58.4 50.6zM320.8 78.4z"/><circle cx="420.9" cy="296.5" r="45.7"/><path d="M520.5 78.1z"/></g></svg>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we remove the index.html changes we'll remove this from the PR as well

AndreEPaul
AndreEPaul requested changes Jan 20, 2021
View reviewed changes
Copy link
Contributor

@AndreEPaul AndreEPaul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall the main changes look fine, there are just a couple things.
I don't think we should be changing all the boilerplate create-react-app stuff like the index.html and whatnot. Deleting the .css files is fine but the rest I just wouldn't take out for now.
Generally I'd say let's try to keep PRs as more granular, this one has 19 file changes which is pretty big.
Only other thing is idk if you want to keep the userview in yours since it's my task, but I'm fine with either way

chungheek
chungheek reviewed Jan 20, 2021
View reviewed changes
front-end/src/App.test.js Outdated
const linkElement = screen.getByText(/learn react/i);
expect(linkElement).toBeInTheDocument();
});
// test('renders learn react link', () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we necessarily need test files, but if you want we can have them, but I would remove any comments

chungheek
chungheek reviewed Jan 20, 2021
View reviewed changes
front-end/src/components/About.js Outdated
</Typography>
</Box>
);
}
Copy link
Member

@chungheek chungheek Jan 20, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add newline

chungheek
chungheek reviewed Jan 20, 2021
View reviewed changes
front-end/src/theme.js Outdated Show resolved Hide resolved
chungheek
chungheek reviewed Jan 20, 2021
View reviewed changes
front-end/src/index.js Outdated Show resolved Hide resolved
@shylton
Update App.js
a29b4b1 
added newline
@shylton
Update App.js
6590707
@shylton
Delete App.test.js
f357304
@shylton
Update About.js
5898c7b 
added newline
@shylton
Update SetSecurityLevel.js
0577541 
removed commented-out code
@shylton
Update SignIn.js
1c82641 
removed commented-out code
@shylton
Update index.js
5014207 
added newline
@shylton
Update theme.js
37d256c 
added newline, rm comments
@shylton
Delete UserView.js
a511506
@shylton
Update App.js
bdf4e51 
rm UserView, will add back later
@shylton
Update secrets.svg
64c7300 
add newline
@shylton
Update NavBar.js
1774a68 
rm User link
shylton
shylton commented Jan 21, 2021
View reviewed changes
Copy link
Contributor Author

@shylton shylton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be all set now?

@AndreEPaul
Copy link
Contributor

Everything looks fine to me except you do have a merge conflict, if you just remove those changes from your PR it should be fine

chungheek
chungheek approved these changes Jan 22, 2021
View reviewed changes
Copy link
Member

@chungheek chungheek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to make a followup PR where it will format/auto-indent based on the linter, but the code is solid good job @shylton ! 👍

AndreEPaul
AndreEPaul approved these changes Jan 22, 2021
View reviewed changes
Copy link
Contributor

@AndreEPaul AndreEPaul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good thanks for making those updates @shylton ! 👍🏾

@chungheek chungheek mentioned this pull request Jan 22, 2021
Merged
@AndreEPaul AndreEPaul mentioned this pull request Jan 22, 2021
Merged
@shylton shylton merged commit 2742506 into main Jan 23, 2021
@shylton shylton deleted the sprint1-login-screen branch January 23, 2021 13:25
AndreEPaul added a commit that referenced this pull request Jan 23, 2021
@AndreEPaul
Merge pull request #6 from cs467-website-security-project/sprint1-use…
55c2b13 
…r-screen

Sprint1 user screen 
(Merge notes:
* rebased after merges of #2 and #5 .
* I had made changes to `SignIn.js` to get my server to run but I rolled them back before merging. 
* Added the maxLength to add secrets modal, thanks @shylton ! )
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chungheek chungheek chungheek approved these changes

@AndreEPaul AndreEPaul AndreEPaul approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@shylton @AndreEPaul @chungheek