Skip to content
This repository was archived by the owner on Oct 2, 2024. It is now read-only.

Scotland class 5 - zoo project#76

Closed
bonboh wants to merge 3 commits intoCodeYourFuture:mainfrom
sSalem123:main
Closed

Scotland class 5 - zoo project#76
bonboh wants to merge 3 commits intoCodeYourFuture:mainfrom
sSalem123:main

Conversation

@bonboh
Copy link
Contributor

@bonboh bonboh commented Jun 13, 2021

No description provided.

Copy link
Contributor Author

@bonboh bonboh left a comment

Choose a reason for hiding this comment

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

Good job @sSalem123! I've left some comments and tips for you to read, but there's nothing else you need to do 👍

</div>
</section>
<section class="panda">
<h3>panda storey</h3>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
<h3>panda storey</h3>
<h2>panda storey</h2>

The other animal sections use h2 so we should use that as well so it has the same heading level

img {
border: 5px solid #8075ff;
}
*/
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Software teams usually don't leave commented code. We can delete the commented code here

}

.giraffe p:first-child {
font-size: 20px;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
font-size: 20px;
font-size: 1.25rem;

We should use relative units where possible, so that if a user changes their browser default font size setting then everything changes size proportionally


/*Education*/

button:hover {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
button:hover {
button:focus,
button:hover {

We should also add :focus so that when a user uses the Tab key to move to a button, it also changes to green


button:hover,
button:focus {
background-color: #F4743B;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We already have button:hover above on line 177 so we can move background-color: #F4743B; there


.membership p:first-child {
width: 700px;
margin: 1rem 1rem 0.5rem 1.5rem;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
margin: 1rem 1rem 0.5rem 1.5rem;
margin: auto;

Alternatively, we can use auto which centres the element. auto distributes the margin around it equally


.programs {
width: 97%;
height: auto;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
height: auto;

We don't need height: auto because auto is the default value of height so it already applies

.programs {
width: 97%;
height: auto;
padding: 10px;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
padding: 10px;
padding: 0.75rem;

We should use relative units where possible, so rem instead of px

@bonboh bonboh closed this Jun 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants