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

Bug: Mobile "follow" menu overlapping issue #179

Merged

Conversation

shredtechular
Copy link
Contributor

This PR fixes #177.

The Good News

The simple bug fix of the mobile "follow" menu overlapping text was actually already identified and fixed in the parent theme already in v4.5.1 . So, this should be as simple as updating the theme in this website to >= v4.5.1, however...

The Bad News

Upgrading the theme to >=v4.5.1 causes some layout issues that appear to have been completed with intent in the parent theme due to certain dependencies of the parent theme being updated. It appears as though these layout changes mostly affect Desktop widths from what I can observe. For example, after an upgrade to v.4.6.0 you can see how the layout for articles change on desktop below:

screen shot 2017-10-24 at 10 02 33 pm

Compare that to the current production site:

screen shot 2017-10-24 at 10 02 48 pm

so you can see how much "thinner" the main content width is. Do you like or don't like that? In my opinion it kinda made the left/right aligned images look weird to me, maybe because they seem bigger?

I'll assume the "thinner" content isn't desired and you would like to utilize cleanly the maximum real estate on the page without so much white space? So, I did include some additional "parent over-ride" CSS for the short term. I'm not really liking doing this, but I'm not sure how else to handle. I can try to create a pull request upstream to the parent theme, but given this was done intentionally, that solution will probably take longer to put together as it will require a lot of theme re-work I'm afraid to try and generalize a compromised solution.

We can also accept the new layout and manually re-size the affected images that don't look great? Maybe that's better than adding over-ride CSS?

Do you have any thoughts, ideas, or preferences on how you would like to handle?

@mtlynch
Copy link
Owner

mtlynch commented Oct 25, 2017

Oof, why'd they make it so thin? I agree with you that it makes the content look weird. I'd rather stick with the current width with overrides. I wish we didn't have to clash with the parent theme, but I can't think of a better solution either.

If we're going with manual overrides, we're ready to merge this in, right?

@shredtechular
Copy link
Contributor Author

Yeah, sorry about that... It seems they made it thinner to accommodate a right hand side bar or "Table of Contents" for posts. Maybe that is another option -- to add a Table of Contents sidebar to posts? I could experiment with that if you'd like, but I think we will still run into the issue with some of the floating images making the content look weird...

And that is where I'd go with a pull request on the parent theme, to add some logic to the layouts that has one set of CSS rules applied if there is a right hand side bar and another set of CSS rules if there was NOT a right hand sidebar to maximize page real estate. I just don't estimate that being a real light lift unfortunately to generalize that.

Anyway, that is correct, these manual CSS overrides are ready to be merged in if you'd like to go with that solution. This solution has been tested across all the usual browsers and device widths, so it should be good to go if you want to roll with that.

@mtlynch
Copy link
Owner

mtlynch commented Oct 25, 2017

Okay, cool. Thanks!

The TOC feature doesn't appeal to me. I like what you've done here, so I'll merge this in.

@mtlynch mtlynch merged commit 5e38670 into mtlynch:master Oct 25, 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.

Mobile: Follow menu covered by article title
2 participants