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 mobile layout: horizontal scrollbars (FF) and td alignment #843

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Vaesper
Copy link
Contributor

@Vaesper Vaesper commented Oct 15, 2016

Okay, once again, please verify that I'm not a complete lunatic and test this on systems I have no access to. Tested: FF 49 and Chromium 53 on linux.

So the HTML fix... Is a complete revert of commit a1f6012. I'm not sure what was going on there, but as far as I can tell there currently is no (and at the time of that commit there was no) white-space: pre applied outside of the td.prefix.

Allowing the whitespace fixes not only horizontal scrollbars on FF with the mobile layout, but ALSO fixes something I'd noticed in both FF and Chromium; the fact that the prefix is kept on the same line as the message, both UNDER the timestamp when the message starts with a long string. This does not make any sense to me, if there's a long unbreakable string at the start of the message (someone posting a link?) I'd want the timestamp and prefix to be on the same line at least.

Long story short, I have no idea what I'm doing but it seems to work. Pls check.

@Vaesper
Copy link
Contributor Author

Vaesper commented Oct 15, 2016

Oh and for sake of clarity; the css changes are to "undo" the vertical-align: top from the desktop layout. For desktop it's necessary to line up the table cells correctly but in mobile (with its display: inline) it looks weird, especially because the time has a smaller font and the prefix a bigger one. To my best estimation probably not a controversial change.

@torhve
Copy link
Member

torhve commented Oct 18, 2016

When I test this PR I get an extra space between the prefix and the line. Maybe that's what the HTML comments fixes? I don't enjoy that extra space.

@lorenzhs
Copy link
Member

Hm appears that way, given the wording of #491?

@Vaesper
Copy link
Contributor Author

Vaesper commented Oct 18, 2016

I think that this whitespace should be attributed to the table-cell padding that is not removed when switching to display: inline in the mobile layout. I will tinker with this PR later and include the CSS fixes from #842 as they do seem to fix FF more thoroughly.

@Vaesper Vaesper force-pushed the bufferlines-on-mobile branch 3 times, most recently from 1ba1f3e to ae8ad4c Compare October 19, 2016 07:35
@Vaesper Vaesper force-pushed the bufferlines-on-mobile branch 2 times, most recently from 9a91643 to ceee0d6 Compare November 11, 2016 14:33
display: block;
flex-wrap: wrap;
Copy link
Member

Choose a reason for hiding this comment

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

you're mixing tabs and spaces here - just a heads up :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

argh, stupid editors. thanks for the heads-up

@Vaesper
Copy link
Contributor Author

Vaesper commented Nov 11, 2016

@torhve you're either going to love me or hate me for what I've done to mobile layout this time :D

@Vaesper Vaesper force-pushed the bufferlines-on-mobile branch 6 times, most recently from 8da7f3f to 166bebe Compare November 18, 2016 13:29
@Vaesper
Copy link
Contributor Author

Vaesper commented Nov 20, 2016

Found some weirdness where the re-ordering of the prefix and timestamp was messed up on some lines on FF mobile. Same bufferline worked fine on FF desktop with the mobile layout, and I have not yet managed to get remote debugging working so not sure what's going on.

@lorenzhs
Copy link
Member

Hey @Vaesper what's the status on this? I've been using it on mobile (iOS) for a while and really like it

@Vaesper
Copy link
Contributor Author

Vaesper commented Mar 15, 2017

My weechat box has been offline for a few weeks due to moving and my lack of time to set it up again, but before that I've been using this branch almost exclusively on my phone and never managed to reproduce that weird timestamp/prefix reordering bug (or even see it again).

It needs to be rebased to master (18 commits behind) and checked that it still works, but github reports no conflicts so at least the rebase should just be a fastforward.

@Vaesper
Copy link
Contributor Author

Vaesper commented Mar 16, 2017

Cannot test it myself right now due to lack of weechat as mentioned previously, but I've gone ahead and rebased the branch. Let me know if things are now (horribly) broken :D

@ghost
Copy link

ghost commented Mar 18, 2017

Tested it, and everything works as expected :)

@lorenzhs
Copy link
Member

I rebased this onto master so I can keep using it on my phone without missing out :)

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