Skip to content

Layout exercise#7

Open
EstefaSora125 wants to merge 4 commits intomasterfrom
feature/layout-exercise
Open

Layout exercise#7
EstefaSora125 wants to merge 4 commits intomasterfrom
feature/layout-exercise

Conversation

@EstefaSora125
Copy link
Copy Markdown
Owner

New layout exercise by Estefania Sora

Comment thread 7-layouts/style.css Outdated
@@ -0,0 +1,135 @@

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change

Comment thread 7-layouts/style.css Outdated


.banner {
align-items: center;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

is this property being used?

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Not really, but I changue te property to justify-content and I can see the change

Comment thread 7-layouts/style.css Outdated
.description_movie {
display: flex;
height: 80%;
width: 100%;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

is this property working?

    width: 100%;

Comment thread 7-layouts/style.css Outdated
Comment on lines +90 to +94
position: sticky;
top:3%;
background-color:#333332;
font-size:x-large;
height: 30px;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

please sort these properties

Comment thread 7-layouts/style.css Outdated
Comment on lines +103 to +105
margin-left: 20px;
margin-right: 40px;
color: white;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

please sort these properties

Comment thread 7-layouts/style.css
}

img {
height: 100%;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

is this property working?

    height: 100%;

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

That property prevent that the images that have a big height size have the same size without write the quantities

Comment thread 7-layouts/style.css Outdated
}

p {
font-family: Georgia, 'Times New Roman', Times, serif;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
font-family: Georgia, 'Times New Roman', Times, serif;

Comment thread 7-layouts/style.css Outdated
margin-top: 2%;
}

p {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
p {
.paragraph {

@juancho11gm
Copy link
Copy Markdown
Collaborator

image

Comment thread 7-layouts/style.css Outdated
Comment on lines +51 to +52
padding-left: 10px;
padding-right: 10px;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
padding-left: 10px;
padding-right: 10px;

Comment thread 7-layouts/style.css Outdated
}

h1 {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change

Comment thread 7-layouts/style.css Outdated
h2 {
font-size: x-large;
width: 70%;
padding-bottom: 6%;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
padding-bottom: 6%;

Comment thread 7-layouts/style.css Outdated
Comment on lines +43 to +46

p {
font-size: 70%;
}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
p {
font-size: 70%;
}

Comment thread 7-layouts/style.css Outdated
Comment on lines +19 to +22
article {
width: 40%;
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
article {
width: 40%;
}

Comment thread 7-layouts/style.css Outdated
Comment on lines +2 to +16
@media screen and (min-width: 545px){
h1 {
font-size: 400%;
}

h2 {
font-size: x-large;
width: 40%;
padding-bottom: 3%;
}

p {
font-size: 130%;
}
}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

is this rule being applied?

Copy link
Copy Markdown
Collaborator

@juancho11gm juancho11gm left a comment

Choose a reason for hiding this comment

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

Hi Estefanía, please see the comments and send the changes. Please comment if you think otherwise

@juancho11gm
Copy link
Copy Markdown
Collaborator

Hi Estefanía, two comments:

  • Use classes instead of elements: you can reuse code. Always prefer classnames over tag names so you avoid side effects and forcing your markup to match your styles.
  • I think that some of your media queries are unnecessary, you only need the mobile, tablet, and desktop breakpoints. Take a look at the example

Mobile
image

Tablet
image

Desktop
image

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.

4 participants