techcorridorio / techcorridorio.github.io

Official site for techcorridor.io
http://www.techcorridor.io/
MIT License
13 stars 9 forks source link

Fix horizontal margins on mobile #32

Closed iLtc closed 7 years ago

iLtc commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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

@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 commented 7 years ago

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 commented 7 years ago

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

iLtc commented 7 years ago

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

Hopefully, it works now.

dahlbyk commented 7 years ago

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

Tip: never open PRs from master. 😀

benjaminoakes commented 7 years ago

Thanks @iLtc!