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

feat:implement responsive learn-more section #34

Merged
merged 3 commits into from
Jul 4, 2020

Conversation

mm1123452
Copy link
Owner

No description provided.

@mm1123452 mm1123452 requested review from julieg18, tantany and Av1sa July 2, 2020 14:07
Copy link
Collaborator

@julieg18 julieg18 left a comment

Choose a reason for hiding this comment

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

We need to change title to "we know a lot". Also the links are made incorrectly, but we could fix that later since it's not about responsiveness.

line-height: 22px;
}
}

@media only screen and (min-width: 769px) and (max-width: 1024px) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

You don't need the and (max-width: ... ) in my opinion

Copy link
Owner Author

Choose a reason for hiding this comment

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

I know

@@ -3,6 +3,13 @@
line-height: 19px;
}

@media only screen and (max-width: 320px) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't have where to write it but, the additional title in 320px screen is 28px and should be 20px.
Also, why in additional info the title block is unique to additional info? why not use content title?

Copy link
Owner Author

Choose a reason for hiding this comment

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

switched

@@ -15,5 +15,6 @@
.additional-info__subtitle {
font-size: 18px;
line-height: 18px;
margin: 22px 0 0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

In 320px screen the size of the subtitle is 12px

Copy link
Owner Author

Choose a reason for hiding this comment

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

switched to content_subtitle


@media only screen and (max-width: 320px) {
.arrow-icon {
font-size: 14px;
Copy link
Collaborator

Choose a reason for hiding this comment

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

The icon of the arrow is aligned to the upper row of the link and not the lower row

Copy link
Owner Author

Choose a reason for hiding this comment

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

updated

.arrow-icon {
font-size: 14px;
width: 26px;
height: 22px;
Copy link
Collaborator

Choose a reason for hiding this comment

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

The arrow is too big compared to the figma design

Copy link
Owner Author

Choose a reason for hiding this comment

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

fixed

@@ -1,11 +1,11 @@
.additional-info__list-item {
margin-bottom: 20px;
margin-bottom: 16px;
Copy link
Collaborator

Choose a reason for hiding this comment

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

You should add "last-of-type", you have extra margin on the second list item

Copy link
Owner Author

Choose a reason for hiding this comment

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

done

@@ -1,11 +1,11 @@
.additional-info__list-item {
margin-bottom: 20px;
margin-bottom: 16px;
Copy link
Collaborator

Choose a reason for hiding this comment

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

margin bottom should be 18px for 320px

Copy link
Owner Author

Choose a reason for hiding this comment

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

done

@@ -3,6 +3,13 @@
line-height: 19px;
}

@media only screen and (max-width: 320px) {
.additional-info__link {
font-size: 14px;
Copy link
Collaborator

Choose a reason for hiding this comment

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

You should specify font-weight everywhere. font-weight 500 looks different the the normal one.

@mm1123452 mm1123452 requested review from julieg18 and tantany July 3, 2020 22:17
Copy link
Collaborator

@julieg18 julieg18 left a comment

Choose a reason for hiding this comment

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

I don't see any issues! Good job!

@mm1123452 mm1123452 merged commit 1e8e654 into develop Jul 4, 2020
@mm1123452 mm1123452 deleted the feature/learn-more-responsive branch July 4, 2020 22:04
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.

3 participants