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.

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

Task - 1 Done by Tahseen Alaa. #2

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

TahseenAlaa
Copy link

Task - 1 Done by Tahseen Alaa.
Added to Solutions Branch.

margin: 50px auto auto 50px;
text-align: center;
left: 20%;
}
Copy link
Member

Choose a reason for hiding this comment

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

header
you don't need just remote it left: 20%. Also you can use short hand property for margin: 50px auto;

}

section {
text-align: center;
Copy link
Member

Choose a reason for hiding this comment

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

useless just remote it

padding: 50px 50px 50px 50px;
left: 27%;
text-align: center;
}
Copy link
Member

Choose a reason for hiding this comment

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

backSpace
as you can see in the image below, your layout is broken!! the reason is left: 27% do you know the purpose of left property? you can't use it everywhere be careful with that. Remove position, text-align Also, use shorthand for margin: 100px auto 50px and padding: 50px
screen shot 2018-03-29 at 22 51 43

background-color: #1ECDE2;
color: #fff;
}

Copy link
Member

Choose a reason for hiding this comment

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

why you are using multiple classes for active buttons ?
you use an .active class instead of these.btn1, .btn2, .btn6, .btn11 and then use it in your button tags!

Copy link
Author

Choose a reason for hiding this comment

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

I thought I will need it later, and forget to delete them.

<div class="">
<button class="btn btn1">C</button>
</div>

Copy link
Member

Choose a reason for hiding this comment

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

remove all <div class=""> since they don't have class!

Copy link
Author

Choose a reason for hiding this comment

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

the whole code need minify before deploy as production.
thanks for the review.
I fixed all bugs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants