Skip to content

Fix: 'Coding Coach' text is coming in a single line. #135

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 13 commits into from
Oct 19, 2018
Merged

Fix: 'Coding Coach' text is coming in a single line. #135

merged 13 commits into from
Oct 19, 2018

Conversation

linux-nerd
Copy link

@linux-nerd linux-nerd commented Oct 7, 2018 •
edited
Loading

Details

Description

  • Changed 'Coding\nCoach' to 'Coding Coach'
  • Added styles from Fix: Main Header Follow-Up: Remove additional <h1> #113 to break the words in multiple lines
  • Removed unused assets/home.scss file
  • Renamed Home.scss to home.scss and moved it inside assets directory.
  • Changed @media screen and (min-width: 768px) to @include respond-to(pc)
  • Added responsive handler for screen size 1024 @media screen and (min-width: 768px) - >
 $break-xlarge: 1024px;
------
 @else if $media==largepc {
    @media (min-width: $break-xlarge) {
        @content;
    }
  }
  • Changed rem to px for padding and margins, considering default pixel size of 16px

Issue

#128

@linux-nerd linux-nerd mentioned this pull request Oct 7, 2018
Merged
5 tasks
crysfel
crysfel requested changes Oct 7, 2018
View reviewed changes
Copy link
Member

@crysfel crysfel left a comment

Choose a reason for hiding this comment

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

Just replace the media queries with respond-to

src/pages/home/assets/home.scss Outdated
background-size: cover;
position: absolute;
z-index: -1;
@media screen and (min-width: 768px) {
Copy link
Member

Choose a reason for hiding this comment

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

Please use the respond-to mixing that we already have: https://github.com/Coding-Coach/coding-coach/blob/development/src/theme/base/_mediaqueries.scss#L36

src/pages/home/assets/home.scss Outdated
padding-top: 6rem;
padding-bottom: 3rem;

@media screen and (min-width: 768px) {
Copy link
Member

Choose a reason for hiding this comment

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

For all media queries use respond-to

@emmabostian
Copy link
Collaborator

I've restarted this build.

@arthurbuhl
Copy link

Build for this PR will still fail, as the build fix (#136) is not merged yet. After merging that PR, you can rebuild again and then this build will pass (if there are no issues in tests, etc)

@crysfel
Copy link
Member

crysfel commented Oct 8, 2018

Please rebase with latest in development

crysfel
crysfel previously approved these changes Oct 9, 2018
View reviewed changes
Copy link
Member

@crysfel crysfel left a comment

Choose a reason for hiding this comment

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

Looks great!

emmabostian
emmabostian previously approved these changes Oct 10, 2018
View reviewed changes
Copy link
Collaborator

@emmabostian emmabostian left a comment

Choose a reason for hiding this comment

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

I'll approve since these are minor, but please fix the units and rebase with development!

src/pages/home/assets/home.scss Outdated
.hero {
position: relative;
min-height: 50vh;
padding-top: 6rem;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please use px for margin & padding.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is in multiple instances, but let's be consistent :)

Copy link
Author

Choose a reason for hiding this comment

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

This is a carry forward from #73 :)
I will fix this :)

@linux-nerd linux-nerd dismissed stale reviews from emmabostian and crysfel via b384df4 October 10, 2018 09:47
crysfel
crysfel approved these changes Oct 10, 2018
View reviewed changes
Copy link
Member

@crysfel crysfel left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks!

emmabostian
emmabostian previously approved these changes Oct 11, 2018
View reviewed changes
crysfel
crysfel requested changes Oct 12, 2018
View reviewed changes
Copy link
Member

@crysfel crysfel left a comment

Choose a reason for hiding this comment

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

Just update the media queries with the latest in development, resolve conflicts and it should be good to go, thansk!

src/theme/base/_mediaqueries.scss Outdated
@@ -69,6 +71,11 @@ $break-large: 992px; // tablets
@content;
}
}
@else if $media==largepc {
Copy link
Member

Choose a reason for hiding this comment

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

With #123 merged, we no longer need this, can you please update your code accordingly? Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

That's why I said in frontend channel that until we got the new mediaqueries.. everyone should have used manual mediaqueries doing mobile-first. Now is going to be a pain in the ass for some people 😞

For any help, reach me on slack (victor ribero)

Copy link
Author

@linux-nerd linux-nerd Oct 16, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

sure 😞

@linux-nerd
Copy link
Author

@crysfel I have somehow merged the development branch and fixed the conflicts 😀

crysfel
crysfel approved these changes Oct 19, 2018
View reviewed changes
Copy link
Member

@crysfel crysfel left a comment

Choose a reason for hiding this comment

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

Great job!

@crysfel crysfel merged commit 7f03ae5 into Coding-Coach:development Oct 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@devictoribero devictoribero devictoribero left review comments

@crysfel crysfel crysfel approved these changes

@emmabostian emmabostian emmabostian left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@linux-nerd @emmabostian @arthurbuhl @crysfel @devictoribero