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

Fix horizontal margins on mobile #32

Merged
merged 7 commits into from Feb 11, 2017
Merged

Conversation

iLtc
Copy link
Contributor

@iLtc iLtc commented Dec 1, 2016

I fixed the horizontal margins on mobile.

about

companies

contact

However, it causes another margin problem on the desktop version.

problem

I can not find an elegant solution. I just override the css attribute.

It looks good now.

image

@dahlbyk
Copy link
Member

dahlbyk commented Jan 7, 2017

I believe the margins can be more simply fixed by adding .col-md-12 to .page-heading in its own row:

    <div class="row">
        <div class="page-header col-md-12">
            <h1>{{ page.header }}</h1>
        </div>
    </div>
    <div class="row">
        <div class="col-md-12">
            {{ content }}
        </div>
    </div>

@iLtc
Copy link
Contributor Author

iLtc commented Jan 8, 2017

Actually, the problem is that there is one .row inside another .row. This is not necessary. After checking the official example, which put .page-header out of the .row, I removed the outer .row.

If you want to keep the .page-header inside the .row, we can either write the two .row separately or remove the inner .row. And you are right, we need add .col-md-12 to .page-header.

Also, this will still cause the margin problem on the desktop version. And we still need to override the css attribute.

@dahlbyk
Copy link
Member

dahlbyk commented Jan 8, 2017

Yeah, I think I have it right with two consecutive rows, one with .page-heading and one for content. I tried the change in Chrome and Firefox dev tools and it seems to work as expected? Not sure why the CSS change would still be required… this is a pretty basic Bootstrap layout.

@iLtc
Copy link
Contributor Author

iLtc commented Jan 9, 2017

Ok, I fixed it by using the two consecutive rows. I also removed the CSS.

Please notice that, on the desktop version, there is a horizontal margin problem now. 😭

screen shot 2017-01-09 at 9 29 03 pm

@benjaminoakes
Copy link
Member

Please notice that, on the desktop version, there is a horizontal margin problem now. 😭

@iLtc Did you want to make any changes to address that?

This does seem better on mobile, so it could make sense to merge now.

@iLtc
Copy link
Contributor Author

iLtc commented Feb 4, 2017

Since I can't solve that, we can merge now.

By the way, this is what I will do next~

Add dynamic "next meetup" to home page

@benjaminoakes
Copy link
Member

Let's move the "next meetup" feature to another PR, since it's off-topic.

@iLtc
Copy link
Contributor Author

iLtc commented Feb 5, 2017

God, I don't know push to my repository will influence here.

Hopefully, it works now.

@dahlbyk
Copy link
Member

dahlbyk commented Feb 11, 2017

God, I don't know push to my repository will influence here.

Tip: never open PRs from master. 😀

@benjaminoakes benjaminoakes merged commit e15609a into techcorridorio:master Feb 11, 2017
@benjaminoakes
Copy link
Member

Thanks @iLtc!

iLtc added a commit to iLtc/techcorridorio.github.io that referenced this pull request Feb 11, 2017
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