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

Responsive design #14

Merged
merged 14 commits into from Jun 19, 2019
Merged

Responsive design #14

merged 14 commits into from Jun 19, 2019

Conversation

bhflm
Copy link
Contributor

@bhflm bhflm commented Jun 13, 2019

Summary

Added responsive style @ index.html for mobile view.

Screenshots

Mobile:
image

Default:
image

Trello Card

https://trello.com/c/5zaWJ1dG/14-dise%C3%B1o-responsive

.book-container {
align-items: center;
flex-direction: column;
height: 520px;

Choose a reason for hiding this comment

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

You don't need to set a height for your containers, you can let the inner content set this for you.

maquetado/scss/styles.scss Outdated Show resolved Hide resolved
@Dorian30 Dorian30 assigned bhflm and unassigned Dorian30 Jun 14, 2019
@@ -13,6 +13,7 @@

.book-container {
display: flex;
flex-direction: row;
background-color: $white;
box-shadow: 4px 4px 10px 0 $black-05;
max-width: 1000px;

Choose a reason for hiding this comment

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

You have margin: 60px auto; set in this class. You shouldn't use margin top in this case to separate this element from the redirect arrow. It's better to set margin-bottom in the arrow itself as margin-bottom: 60px;

}

.redirect-arrow {
margin-left: 0;

Choose a reason for hiding this comment

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

As mentioned above, the redirect arrow should have margin: 0 0 60px 100px; instead of setting margin: 60px auto; in your book-container class. Then set margin: 0; here in the media query.

maquetado/scss/styles.scss Show resolved Hide resolved
@@ -13,6 +13,7 @@

.book-container {
display: flex;
flex-direction: row;

Choose a reason for hiding this comment

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

Remember the alphabetical order.

box-shadow: 4px 4px 10px 0 $black-05;
flex-direction: column;
max-width: 330px;
margin: 30px auto;

Choose a reason for hiding this comment

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

Same, you should only use margin-bottom: 30px; to separate one element from another and padding for positioning inner elements. The auto part is not need since you are already centering your content with align-items: center;

left: 220px;
position: absolute;
top: 20px;
transform: rotate(15deg);

Choose a reason for hiding this comment

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

The following properties are already set outside the media query:

content: '';
background: url('./assets/book-badge.png') no-repeat center/contain;
left: 220px;
position: absolute;
transform: rotate(15deg);

Don't re write them unless you're going to override them for other value


.book-info-container {
flex-grow: 0;
margin-right: 0;

Choose a reason for hiding this comment

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

Use padding instead to separate an inner element from its parent. Use margins only to separate siblings

image

}

.book-title-container {
margin-bottom: 0;

Choose a reason for hiding this comment

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

Be careful with these things. You are separating the title from the content through the title's margin bottom and it should the parent book-title-container the one doing that.

image


.book-title {
font-size: 20px;
margin-right: 10px;

Choose a reason for hiding this comment

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

Same comment. This element has top and bottom margins. Set them to 0 outside this media query and use the title container to set the margin bottom to separate the title from the content

maquetado/scss/styles.scss Show resolved Hide resolved
@Dorian30 Dorian30 removed their assignment Jun 15, 2019
@@ -56,3 +56,79 @@
display: flex;
flex-direction: row;
}

@media only screen and (max-width: 1024px) {

Choose a reason for hiding this comment

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

remove empty line

maquetado/scss/styles.scss Show resolved Hide resolved
@@ -39,7 +39,7 @@

.redirect-arrow {
font-size: 20px;
margin-left: 100px;
margin-left: 0 0 60px 100px;

Choose a reason for hiding this comment

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

You are using a shorthand. This should be

Suggested change
margin-left: 0 0 60px 100px;
margin: 0 0 60px 100px;

maquetado/scss/books.scss Show resolved Hide resolved
@Dorian30 Dorian30 removed their assignment Jun 19, 2019
@bhflm bhflm merged commit f4d511c into signup Jun 19, 2019
@bhflm bhflm deleted the responsive branch July 1, 2019 20:18
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

3 participants