-
Notifications
You must be signed in to change notification settings - Fork 41
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
P10 Added judging critera page #66
P10 Added judging critera page #66
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.
@andrewdempsey2018 this is a really good first pull request you can be proud of! Just a few minor changes that I would recommend. Once you made the changes, just tag me or Tim in the Pull Request again and we can approve it. For future use I would also recommend to flesh out the Pull Request message a little bit (e.g. add areas such as Description, Tests, Errors, Screenshots, etc.); we will also add a template shortly as well.
User Story ID: #P10 Description of PR: Tests:
Screenshots: Desktop Firefox test: Desktop Chrome test: Emulated smartphone test: Know bugs/errors:
Post review/additional changes:
|
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.
LGTM
Just FYI - not sure if you are aware, but you can edit the PR message body. Also, usually if somebody leaves comments, you would use the Reply option for each comment and Resolve the conversation once the comment is addressed. Not a biggie though.
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.
LGTM - might get @stefdworschak to just check from his end.
Also - there might be some issues, since @JimLynx was working on the footer, and the view/url for this was different.
I do like @andrewdempsey2018 namespace better though, short and sweet.
Actually, @andrewdempsey2018 - can you have a look at #74 and adjust the urls/views accordingly to match that of @JimLynx please? |
|
Noted, thanks for the tip. |
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.
LGTM
Added basic "Judging Criteria" page with some placeholder text. Setup urls, view and navbar link to access the page.