Skip to content

Conversation

@EstefaSora125
Copy link
Owner

New layout exercise by Estefania Sora

@@ -0,0 +1,135 @@

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change



.banner {
align-items: center;
Copy link
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
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

.description_movie {
display: flex;
height: 80%;
width: 100%;
Copy link
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 on lines 90 to 94
position: sticky;
top:3%;
background-color:#333332;
font-size:x-large;
height: 30px;
Copy link
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 on lines 103 to 105
margin-left: 20px;
margin-right: 40px;
color: white;
Copy link
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

}

img {
height: 100%;
Copy link
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
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

}

p {
font-family: Georgia, 'Times New Roman', Times, serif;
Copy link
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;

margin-top: 2%;
}

p {
Copy link
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
Collaborator

image

Comment on lines 51 to 52
padding-left: 10px;
padding-right: 10px;
Copy link
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;

}

h1 {

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change

h2 {
font-size: x-large;
width: 70%;
padding-bottom: 6%;
Copy link
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 on lines 43 to 46

p {
font-size: 70%;
}
Copy link
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 on lines 19 to 22
article {
width: 40%;
}

Copy link
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 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
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
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
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.

5 participants