Remove table layout for chat messages (and fix layout issues yet again) #523

maxpoulin64 · 2016-07-23T02:05:57Z

Fixes this:

back into this:

EDIT: So, this still had bugs, so I removed the table layout entirely. This one seems to just play nice now.

astorije · 2016-07-23T04:26:59Z

I had played with removing the table layout for a bit, and after noticing so many bugs, I simply canceled. I need to test this PR thoroughly before we can merge or we'll do like we've been doing for a few weeks: one step forward, N steps backwards.

xPaw · 2016-07-23T16:44:05Z

Just wondering, what happens with very long names or notices? How do they overflow

maxpoulin64 · 2016-07-23T19:05:25Z

@xPaw: they wrap on multiple lines. Worse case they push the column for that one message, which is slighly uglier but has the advantage of not fucking up the whole table, so I think it's worth it anyway. I didn't change the wrapping code, so if there's something it probably was already there.

astorije · 2016-07-25T02:41:34Z

which is slighly uglier but has the advantage of not fucking up the whole table, so I think it's worth it anyway.

Do you have any screenshot of this by any chance?

maxpoulin64 · 2016-07-25T03:23:12Z

which is slighly uglier but has the advantage of not fucking up the whole table, so I think it's worth it anyway.

Do you have any screenshot of this by any chance?

No, I haven't managed to trigger it yet. So far it seems like it does the same as it did with the table, the nick will just wrap on a second line. But if it were to happen, it would look a bit like this:

But it looks like this instead (but it also does on master):

I can also confirm long messages such as long links wrap properly and don't overflow.

williamboman · 2016-08-07T14:38:30Z

Looks like this messes up existing border color on non-default themes (tested only on Zenburn).

xPaw · 2016-08-20T12:23:48Z

What about using flexbox instead of floats? The textarea already does.

astorije · 2016-08-20T12:49:38Z

Just curious, why removing the (CSS) table layout? I didn't know much about the use cases for this but it looks like it matches our goal fairly well, no?

maxpoulin64 · 2016-09-04T00:07:03Z

@astorije because it keeps exploding, especially with the unread marker? See the screenshot in the first post and try to tell me this is normal, expected layout for it. It's horrible.

Good point for the flexbox, will definitely go with that option.

maxpoulin64 · 2016-09-04T01:31:23Z

Updated to flexbox

AlMcKinlay · 2016-09-05T11:35:01Z

I'm going to stick this PR on my running version and see how it works over the next few days. That's how I found all of the bugs in @astorije's one. But this looks promising, that's for sure.

astorije · 2016-09-06T05:33:54Z

That's how I found all of the bugs in @astorije's one.

🔪💔😭 ... 😄

See the screenshot in the first post and try to tell me this is normal, expected layout for it. It's horrible.

Agreed, that's gross.
I'll test this as well. Would be pretty awesome if we can get this sh*t behind us once and for all!

maxpoulin64 · 2016-09-06T05:48:01Z

That's how I found all of the bugs in @astorije's one.

That's how I test mines 😛 Not uncommon that I do a bunch of updates after opening the PR

AlMcKinlay · 2016-09-07T08:22:16Z

Found my first bug.

Copy a line on the chat
Paste it into the textarea
You get 3 lines: the time on 1, the username on the next, and the message on the final one.

astorije · 2016-09-07T13:05:53Z

Is that a big bug though?
Worst case, can we intercept the copy to put them back into one line? It feels hackish but I don't think there is another option. More importantly, it could let us improve the copy and paste experience, such as wrapping nick with <> and padding the nicks when copying multiple lines.

AlMcKinlay · 2016-09-07T13:41:18Z

Is that a big bug though?

It's big enough that I wouldn't be ok with 👍 this while it still exists.

can we intercept the copy to put them back into one line

That would be an acceptable solution, yeah. I had thought about doing this for exactly what you suggested at some point, but never did it.

What I would really like to figure out is why is it putting in newlines in the first place?

astorije · 2016-09-07T14:43:01Z

What I would really like to figure out is why is it putting in newlines in the first place?

Most likely because of this switch. Again, I don't think there is anything we can do without breaking the layout again. Maybe try with inline-block?

That would be an acceptable solution, yeah.

Right, but definitely for another PR.

astorije · 2016-09-12T05:34:35Z

@maxpoulin64, given @YaManicKill's last comment and the fact that the 2.0.0 issue list is never ending (we have had 8-12 open issues/PRs there forever now...), do you think we could move that to the next release?
Current master works fine for v2.0.0 I think. Definitely looking forward to extensively test this once the 2.0.0 list is empty though.

astorije

Awesome stuff, @maxpoulin64!!
Only a few things I believe, some questions to be answered and one small thing I noticed (that margin: auto; thingy) but hopefully this is going to be the end of an era of bugs!

I also think that all the word-wrap and word-break and overflow: hidden we put all over the place to counter the effects of the table layout are going to be needed anymore. Wanna take a stab at looking at which ones can be removed?

astorije · 2016-09-20T04:32:30Z

client/css/style.css

 	padding: 10px 0;
 }

 #chat .msg {
 	word-wrap: break-word;
 	word-break: break-word; /* Webkit-specific */
+	display: flex;
+	overflow: hidden;
+	position: relative;


I could not figure out why these last 2 lines (overflow: hidden; and position: relative;) are necessary. It seems like disabling them doesn't do anything for me.
Do you remember, by any chance?

astorije · 2016-09-20T04:34:10Z

client/css/style.css

 }

 #chat .time {
 	color: #ddd;
 	text-align: right;
-	max-width: 46px;
-	min-width: 46px;
+	width: 46px;


I remember I had to set these 2 explicitly, but I can't remember why specifically: 8118d56#diff-97db1f70168fb5f12457b238ff6052b5L771

Chances are, this was table-layout-specific anyway...

astorije · 2016-09-20T04:35:32Z

client/css/style.css

 }

 #chat .from {
 	border-right: 1px solid #f6f6f6;
 	color: #b1c3ce;
 	padding-right: 10px;
 	text-align: right;
-	max-width: 134px;
-	min-width: 134px;
+	width: 134px;


Same here, :shrug_emoji:
3cddbbc#diff-97db1f70168fb5f12457b238ff6052b5L754

I guess this is why we should really add some comments to our code...

astorije · 2016-09-20T04:37:31Z

client/css/style.css

-	max-width: 134px;
-	min-width: 134px;
+	width: 134px;
+	align-self: stretch;


I can't seem to see a change when I enable/disable this. Remember why?

According to https://css-tricks.com/snippets/css/a-guide-to-flexbox/, stretch is the default value of align-items, and align-self is used to override it. Since this is not using align-items, there is nothing to override. Removing.

astorije · 2016-09-20T04:41:34Z

client/css/style.css

+
+#chat .text {
+	margin: auto;
+	overflow: hidden;


I believe this is unnecessary as it is shadowed by L907/R907 anyway.
Actually, that whole block seems unnecessary with this flexbox layout now:

#chat .msg.motd .text, #chat .msg.message .text, #chat .msg.action .action-text, #chat .msg.notice .text { white-space: pre-wrap; overflow: hidden; }

Playing with this more, I can't trigger something that needs it (at least on Chrome). Removing both.

astorije · 2016-09-20T04:44:43Z

client/css/style.css

+}
+
+#chat .text {
+	margin: auto;


Not sure the specific reason for this, but this is adding a top margin when text is one line but "from" goes over multiple lines. Example:

margin: auto; ~~margin: auto;~~

I have been trying to fix this and none of align-items: flex-start;, align-content: flex-start; or self-align: flex-start; did the trick.
I guess I'll just remove margin: auto unless someone has reasons to believe this is going to break something else.

astorije · 2016-09-21T04:18:11Z

client/css/style.css

 	padding: 10px 0;
 }

 #chat .msg {
 	word-wrap: break-word;
 	word-break: break-word; /* Webkit-specific */
+	display: flex;


@Gilles123 found out that these still need prefixing for now: #624 (comment)
Mind adding them to this PR please? :)

with autoprefixer and #609 this could be done magically behind the scenes :)

nornagon · 2016-09-25T02:34:09Z

FYI, not directly related to this PR but still relevant: with flex-direction: column-reverse on the message list, we can almost entirely drop the stickyscroll jQuery plugin. column-reverse changes the way scrolling behaves when the DOM changes to consider the bottom of the element the "sticky" part, so adding a new message automatically scrolls the div down—no JS required! You still need a little JS (~a few lines) to keep the scroll position stuck if a message is added while you're scrolled up, but it's much simpler.

nornagon/lounge@react...nornagon:react-chat#diff-97db1f70168fb5f12457b238ff6052b5R768 if you're curious.

xPaw · 2016-09-25T10:00:47Z

@nornagon That makes selecting messages weird. And copying the messages makes them backwards.

nornagon · 2016-09-25T21:35:27Z

@xPaw oh huh, i missed that! weird, that seems like a bug :/

xPaw · 2016-10-01T14:34:12Z

To change how nicks overflow (instead of jumping to new line, extend it to right, this CSS needs to be added:

#chat .from {
    padding-left: 10px;
    min-width: 134px;
}

astorije · 2016-10-11T13:00:55Z

@maxpoulin64, I know you're locally running an older version of that PR, before you moved to flexbox. Do you mind updating your instance to run the latest version, so it lives in prod somewhere before merging?

I'd love to ship this, but we need to make sure the issues highlighted above are fixed beforehand.

astorije · 2016-12-09T05:40:11Z

@maxpoulin64, any chance you could be giving another stab at this?

xPaw · 2016-12-10T20:27:23Z

@astorije can you take this over?

astorije · 2016-12-11T09:32:00Z

@astorije can you take this over?

Will do. Do you have a list of requirements you think are important for this on the top of your head?

xPaw · 2016-12-11T09:39:50Z

@astorije just the stuff me and you pointed out.

astorije · 2017-03-29T04:21:35Z

This PR was 370 commits behind 😱

I have rebased it and I'm quickly testing it, but if someone (@thelounge/maintainers? @williamboman? @MaxLeiter?) would like to give it a try by merging it on their prod instance to have it run over multiple days, that would be very useful.

In general, does it seem like a good idea to switch from a CSS table layout (this is not evil, calm down 😅) to flexbox? What do we gain and lose?

williamboman · 2017-03-29T09:01:03Z

I've run this on and off multiple times since it was opened, never ran into issues with it (other than it breaking other PRs such as #759). LGTM

astorije · 2017-03-30T23:54:03Z

@YaManicKill, @williamboman, I have made a couple changes and rebased with master. Do you mind testing this a bit on your instances? Thanks!

astorije

As discussed on the channel with @xPaw, let's get this merged and deal with the fallout post-merge until we cut a release.
8 months to merge this, sorry @maxpoulin64 😅

Remove table layout for chat messages (and fix layout issues yet again)

maxpoulin64 added Type: Feature Tickets that describe a desired feature or PRs that add them to the project. second review needed labels Jul 23, 2016

maxpoulin64 added this to the 2.0.0 milestone Jul 23, 2016

maxpoulin64 force-pushed the PR/fix-msg-width branch from 58c8a6a to c82887d Compare July 23, 2016 02:28

maxpoulin64 changed the title ~~Fix weird unneeded wrapping introduced by the unread marker~~ Remove table layout for chat messages (and fix layout issues yet again) Jul 23, 2016

maxpoulin64 force-pushed the PR/fix-msg-width branch 2 times, most recently from 5787c5b to e8f42ca Compare July 23, 2016 02:36

astorije self-assigned this Jul 23, 2016

maxpoulin64 mentioned this pull request Aug 8, 2016

Long URLs overflow the chat window and create a transparent yet present horizontal scrollbar #550

Closed

astorije mentioned this pull request Aug 20, 2016

Allow long URLs to break onto next line on Chrome #576

Merged

maxpoulin64 force-pushed the PR/fix-msg-width branch from e8f42ca to 045e0cf Compare September 4, 2016 01:31

astorije mentioned this pull request Sep 12, 2016

Improve inline previews for links and images #524

Merged

astorije added the Meta: Do Not Merge This PR should not be merged. label Sep 18, 2016

astorije force-pushed the PR/fix-msg-width branch from 045e0cf to 9dac932 Compare September 20, 2016 04:17

astorije requested changes Sep 20, 2016

View reviewed changes

astorije reviewed Sep 21, 2016

View reviewed changes

astorije removed the Meta: Do Not Merge This PR should not be merged. label Sep 25, 2016

xPaw removed the second review needed label Dec 17, 2016

xPaw added this to the 2.3.0 milestone Dec 17, 2016

williamboman mentioned this pull request Mar 22, 2017

Initial status message condensing #759

Merged

15 tasks

astorije force-pushed the PR/fix-msg-width branch from 9dac932 to 13ef21e Compare March 29, 2017 04:18

maxpoulin64 and others added 2 commits March 30, 2017 19:50

Remove table layout for chat messages (and fix layout issues yet again)

35b6b47

Improvements to the new flex layout for messages

fb672ab

astorije force-pushed the PR/fix-msg-width branch from baf987e to fb672ab Compare March 30, 2017 23:50

astorije approved these changes Apr 1, 2017

View reviewed changes

astorije merged commit 953325a into master Apr 1, 2017

astorije deleted the PR/fix-msg-width branch April 1, 2017 06:40

This was referenced Jun 16, 2017

Fix the copying of multiple lines having too many linebreaks #1227

Closed

Multiple spaces in messages not shown as such #1229

Closed

matburnham pushed a commit to matburnham/lounge that referenced this pull request Sep 6, 2017

Merge pull request thelounge#523 from thelounge/PR/fix-msg-width

6cb1fcd

Remove table layout for chat messages (and fix layout issues yet again)

Remove table layout for chat messages (and fix layout issues yet again) #523

Remove table layout for chat messages (and fix layout issues yet again) #523

Conversation

maxpoulin64 commented Jul 23, 2016 • edited Loading

astorije commented Jul 23, 2016

xPaw commented Jul 23, 2016

maxpoulin64 commented Jul 23, 2016

astorije commented Jul 25, 2016

maxpoulin64 commented Jul 25, 2016

williamboman commented Aug 7, 2016

xPaw commented Aug 20, 2016

astorije commented Aug 20, 2016

maxpoulin64 commented Sep 4, 2016 • edited Loading

maxpoulin64 commented Sep 4, 2016

AlMcKinlay commented Sep 5, 2016

astorije commented Sep 6, 2016

maxpoulin64 commented Sep 6, 2016

AlMcKinlay commented Sep 7, 2016

astorije commented Sep 7, 2016

AlMcKinlay commented Sep 7, 2016 • edited Loading

astorije commented Sep 7, 2016

astorije commented Sep 12, 2016

astorije left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astorije Sep 20, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nornagon commented Sep 25, 2016

xPaw commented Sep 25, 2016

nornagon commented Sep 25, 2016

xPaw commented Oct 1, 2016

astorije commented Oct 11, 2016 • edited Loading

astorije commented Dec 9, 2016

xPaw commented Dec 10, 2016

astorije commented Dec 11, 2016

xPaw commented Dec 11, 2016

astorije commented Mar 29, 2017

williamboman commented Mar 29, 2017 • edited Loading

astorije commented Mar 30, 2017

astorije left a comment

Choose a reason for hiding this comment

maxpoulin64 commented Jul 23, 2016 •

edited

Loading

maxpoulin64 commented Sep 4, 2016 •

edited

Loading

AlMcKinlay commented Sep 7, 2016 •

edited

Loading

astorije Sep 20, 2016 •

edited

Loading

astorije commented Oct 11, 2016 •

edited

Loading

williamboman commented Mar 29, 2017 •

edited

Loading