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

connect message spacing fix #1259

Merged
merged 6 commits into from Jan 10, 2019
Merged

Conversation

mariocsee
Copy link
Contributor

@mariocsee mariocsee commented Dec 4, 2018

What type of PR is this?

  • Bug Fix

Description

It seems like content in <code> and <pre> took precedence over other HTML elements in Connect. Rather than using the previous approach and forcing breaks in the line, I have added width assertions in CSS to prevent the channel list on the left and the channel content on the right from being squished. The code blocks and messages are capped and adjust to the opening and closing of content on the right.

One notable change I have made is that in mobile, is that when right content is opened, rather than pushing all others to the left, I decided to overlay it on top of the chat preventing a horizontal scroll from appearing on mobile pages.

Related Tickets & Documents

resolves #1256 and resolves #1257

Mobile & Desktop Screenshots/Recordings

Previous behavior in #1256 and #1257

Current behavior (desktop and tablet):

2018-12-22 02 14 27

Firefox word-break:

screen shot 2019-01-08 at 17 18 29

Current behavior (mobile):

2018-12-22 02 23 11

Added to documentation?

  • no documentation needed

What gif best describes this PR or how it makes you feel?

interstella 5555

@benhalpern
Copy link
Contributor

Hmmm I think <pre> blocks should have horizontal scrolling as opposed to line breaks. I think this is the expected behavior with code.

@mariocsee
Copy link
Contributor Author

mariocsee commented Dec 5, 2018

Gotcha, I'll adjust styling for <pre> so that overflow horizontal scrolling happens so that the channel details and sidebar width isn't affected.

@benhalpern What are your thoughts on break-word to break-all so that strings on Firefox in #1257 are chopped (but then it breaks any word that isn't really long)? Should I instead adjust for overflow and allow horizontal scrolling?

@benhalpern
Copy link
Contributor

For normal text we should not have scrolling and we should prefer to break words. I'm not totally sure about #1257 but don't see it as a critical issue to fix. Long words should be broken. I'm not sure about the details on how to do that exactly.

Horizontal scrolling should only apply to <pre> tags, just as it does in articles/comments on the site,

e.g.

https://dev.to/deciduously/comment/7bo3

@mariocsee
Copy link
Contributor Author

mariocsee commented Dec 11, 2018

Somehow the way Firefox handles break-word doesn't actually chop particular words into shorter segments, so I can't address #1257 without accepting the repercussions of using break-all.

After a lot of tinkering around, my hunch is that the problem of the <pre> code blocks in chat vs elsewhere is that its width isn't hard capped at a maximum, which explains why overflow-x doesn't seem like its working in chat but works in comments and articles. The length/size of its content in both <pre> and <code> always takes precedence over the determined max-width. In addition, individual code lines aren't wrapped in a <pre> tag like code blocks are and cannot be capped with max-width + overflow-x.

width assertions
@mariocsee mariocsee changed the title [WIP] connect message spacing/line break fix [WIP] connect message spacing fix Dec 18, 2018
@mariocsee
Copy link
Contributor Author

I just made some changes to the doc above and updated behavior! Focused on asserting width caps to prevent squeezing of other elements.

@mariocsee mariocsee changed the title [WIP] connect message spacing fix connect message spacing fix Jan 7, 2019
@pr-triage pr-triage bot added the PR: unreviewed bot applied label for PR's with no review label Jan 7, 2019
@mariocsee
Copy link
Contributor Author

@benhalpern Just a progress update. All the changes I made are in css and mostly concerns adding a lot of min and max widths and flexbox to prevent too much squeezing and stretching of elements.

One major change is how additional content like user details and articles appear on mobile:

  • currently, it pushes past the chat window to the right and creates a horizontal scroll, which I found not the most appealing experience
  • my suggested alternative puts the content on top of the chat to prevent extending to the right and having the user scroll to the right before seeing the content fully

Let me know your thoughts on this when you get the chance to see and go through it.

@pr-triage pr-triage bot added PR: reviewed-approved bot applied label for PR's where reviewer approves changes and removed PR: unreviewed bot applied label for PR's with no review labels Jan 10, 2019
@benhalpern benhalpern merged commit 6ade962 into forem:master Jan 10, 2019
@pr-triage pr-triage bot added PR: merged bot applied label for PR's that are merged and removed PR: reviewed-approved bot applied label for PR's where reviewer approves changes labels Jan 10, 2019
@mariocsee mariocsee deleted the connect-message-spacing branch January 21, 2019 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: merged bot applied label for PR's that are merged
Projects
None yet
2 participants