-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Update notmyidea theme to scale down to smaller screens. #2914
Conversation
Here's the Google Mobile-Friendly Tester rating the modified theme as mobile-friendly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello,
Thank you for working on this!
As a general note, I would expect all the commits to be squashed into a single one, the upstream doesn't need your development history I think. Some additional comments follow.
Hello.
My commit: |
@paulfertser thanks. I don't use GitHub a lot for collaboration myself, but I believe there's been a "merge and squash" button available to maintainers for a few years now for use at their discretion. Let me know if that's not the case. |
Ben, you might be right, I really have little clue about Github-specific ways on using Git, sorry if I'm misguiding. Including two screenshots from Chromium on Debian which you might consider to view as bug reports. |
BTW, I was told that adding some horizontal padding makes it look better on narrow screens:
|
I've now updated the screenshots in the merge request description. Hehe @paulfertser, either you caught me mid-upload or there's some weird dark theme thing going on in your screenshot above. I suspect that's not an issue with the changes. :) |
The dark theme is indeed on my client side, it's the integrated (provided by chromium without additional addons) mechanism, please consider being compatible with it. I also usually disable javascript so the blog title might be looking different (and overlapping) because of that. |
Thanks, I'm sure it may, but I'm specifically aiming to keep the changes as minimal as possible. There's endless amounts of tweaking that could be done to refine the layout. My aim is for a small change that makes mobile work reasonably well. |
If it's ok with you, I'll treat dark theme support as out of scope for this change. This is specifically about better behaviour on smaller screens. |
The dark theme is indeed on my client side, it's the integrated (provided by
chromium without additional addons) mechanism, please consider being
compatible with it. I also usually disable javascript so the blog title might
be looking different (and overlapping) because of that.
If it's ok with you, I'll treat dark theme support as out of scope for this
change. This is specifically about better behaviour on smaller screens.
Sure thing, thank you for working on this! The title looks fine with
JS disabled now too. Awesome! :)
|
Thanks @Arseney300, I've fixed the header spacing you mentioned. I agree that there's a lot that could be done to further refine the layout, but this change is just intended to provide basic support for small screens with minimal changes. I'd suggest working on further refinements to the post-meta and footer links once this basic support is merged in. |
The aim here is to make the theme work respectably on mobile devices with only modest changes. Providing different layouts at multiple breakpoints is beyond the scope of this change. The changes here are: 1. `base.html`: adding a `<meta name="viewport"` element 2. `main.css`: * use "max-width" instead of "width" * set a "line-height" an the banner and adjust vertical spacing to match * remove fixed height on the nav bar and force it to contain its child elements
Many thanks for your work on this, Ben. And thanks to Paul and Arseney for chiming in as well. 🥇
I'm not sure I agree. When formulated with thought and care, multiple commits can indeed be quite useful. Also, during feedback-and-change iterations, it can be useful to see what has changed in a PR over time, in which case cleaning up the commits can be done at the very end, right before merging.
That is correct. If the commits aren't valuable on their own (e.g., if there are "fix-up" commits that add no value as separate commits), the maintainer can squash the commits if the contributor hasn't already done said clean-up. |
I've just updated the remaining test HTML files for the locales that were skipped on my machine. I'm having some trouble getting these additional locales to work, so I'll check back in tomorrow to see if they passed CI. |
@justinmayer would you mind kicking off the CI process please so I can see if we get a clean test run for all locales? Thanks! |
Hi @justinmayer, I'm having some trouble figuring out whether there are test errors related to my changes or not. It looks like the errors I'm seeing on my machine are due to a missing empty At this point I'm a bit stuck. Can you advise on how best to proceed? |
@BenSturmfels It's probably related to the newly released |
@avaris is there any chance you or one of the team could do that regeneration outside of this pull request? I'd really appreciate it. I'm having trouble with the locales and I'm concerned that otherwise this pull request will get bogged down in unrelated test issues I'm not very familiar with. |
Signed-off-by: Deniz Turgut <dturgut@gmail.com>
I added to this PR, hope you don't mind :). |
@avaris thank you! Doesn't bother me to have it in this pull request. Of course I'm not the one that has to review it though. ;) Thanks again. |
Is there anything I can do to help this change be merged? It looks a lot bigger than it really is - really only an additional |
@avaris: The more I look at the empty @BenSturmfels: I haven't had a chance to look at how differently these changes render the output, but the code-level changes look fine to me. Anyone else have any comments on these changes? |
@justinmayer I think you are correct. I didn't check the diffs that closely (my bad). |
I just released FeedGenerator 2.0.0 containing a fix for the double subtitle elements, so I dropped the commit @avaris added to this PR and merged the other three commits manually. Many thanks to @BenSturmfels for these improvements, which have been requested for quite some time. Truly much appreciated! 🥇 |
The aim here is make the theme work respectably on mobile devices with only modest changes. Providing different layouts at multiple breakpoints is beyond the scope of this change.
The changes here are:
base.html
: add a<meta name="viewport"
elementmain.css
:The test changes just duplicate these changes to
base.html
andmain.css
.Here's an example running under Chrome responsive view:
This theme is also currently running at https://stumbles.id.au/.
Pull Request Checklist