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 wrapping of long room topics (and overlap with apps) #5549

Merged
merged 4 commits into from Nov 9, 2017

Conversation

@rxl881
Copy link
Contributor

rxl881 commented Nov 8, 2017

.mx_RoomHeader_wrapper height needs to be increased to prevent max height of child elements from overflowing allocated space (.mx_RoomHeader_topic.max-height: 42px + .mx_RoomHeader_name.height: 31px == 73px total). Wrapper element height is currently set to 70px.

Setting a column width on the room topic causes overflow to be wrapped to another column (right) and hidden (by existing "overflow: hidden", directive).

@rxl881 rxl881 requested review from ara4n and lukebarnard1 Nov 8, 2017
@lukebarnard1

This comment has been minimized.

Copy link
Contributor

lukebarnard1 commented Nov 8, 2017

So the column-width looks great. I'm not convinced we need to change the height of the wrapper, especially as it's already out of line with the RightPanel header. Upon investigation, it seems that

.mx_AppsDrawer {
    margin-bottom: 3px;
}

is causing the room header to be 3px taller than it should be, which is odd as the app draw was closed.

I'm not sure why the topic has a max-height tbh.

@turt2live

This comment has been minimized.

Copy link
Member

turt2live commented Nov 8, 2017

(the 4px thing is mentioned in #4981 fwiw)

@rxl881

This comment has been minimized.

Copy link
Contributor Author

rxl881 commented Nov 8, 2017

Thanks for linking to and reporting the other issue @turt2live. The first part of this (the 3px of text from the top of the wrapped line) should now be fixed:
Before -
screen shot 2017-11-08 at 17 50 31

After -
screen shot 2017-11-08 at 17 50 44

I'm a little confused by both of your (slightly differing) reports that there is a problem with the margin or padding of the widgets drawer / tray (would be helpful to have the CSS class / id of the element that you think is a problem in the other issue report @turt2live).

In this case, I am sure that the problem is with .mx_RoomHeader_wrapper. The class .mx_RoomHeader_wrapper has a fixed height of 70px. It contains mx_RoomHeader_name with a fixed height of 31px, and mx_RoomHeader_topic with a max-height of 42px (totalling a hight of 73px, inside a container with a max height of 70px). In the chrome DOM inspector, you can see the child content overflowing onto the content below it:

screen shot 2017-11-08 at 17 59 43

screen shot 2017-11-08 at 18 00 14

An alternative solution to increasing the size of the wrapper is decreasing the max-height of the mx_RoomHeader_topic by 3px.

@lukebarnard1

This comment has been minimized.

Copy link
Contributor

lukebarnard1 commented Nov 8, 2017

An alternative solution to increasing the size of the wrapper is decreasing the max-height of the mx_RoomHeader_topic by 3px.

Increasing the size of the header will only make the mis-alignment with RightPanel worse.

When the app draw is not visible, it shouldn't take up any height. But it is, as described in #4981 (comment).

(would be helpful to have the CSS class / id of the element that you think is a problem in the other issue report @turt2live).

.mx_AppsDrawer is the one you want (as mentioned previously).

…iles.

Comment unused classes.
margin-bottom: 3px;
}
// .mx_AppsDrawer {
// }

This comment has been minimized.

Copy link
@rxl881

rxl881 Nov 8, 2017

Author Contributor

I have intentionally left these commented classes for the time being.

@rxl881

This comment has been minimized.

Copy link
Contributor Author

rxl881 commented Nov 8, 2017

Urrghh... I think that we are suffering from some miscommunication here... and are dealing with a couple of (slightly) different issues.

You're absolutely right, there is a margin-bottom, 3px on the .mx_AppsDrawer that shouldn't be there when there are no apps / the apps drawer is not visible. I'll call this issue #2 as it is unrelated to the issue that I was originally trying to solve. I have now fixed this on the most recent commit (by moving the margin to the AppTile elements). This fixes the alignment issues with the left and right columns when no apps are present.

Issue #1 (that this PR is originally about) is still relevant; content overflows .mx_RoomHeader_wrapper due to static height specifications on the contained elements. Regardless of the margin on the apps drawer this causes long topic content to overlap other elements, e.g. the AppsDrawer when apps are present.

@lukebarnard1:

An alternative solution to increasing the size of the wrapper is decreasing the max-height of the mx_RoomHeader_topic by 3px.

Increasing the size of the header will only make the mis-alignment with RightPanel worse.

As mentioned in my comment, decreasing the max-height will fix the problem without impacting the mis-alignment. I've updated the PR to reflect / implement this.

@lukebarnard1, @ara4n - Can you please check that you're happy, and that this is good to go? Thanks.

@lukebarnard1

This comment has been minimized.

Copy link
Contributor

lukebarnard1 commented Nov 9, 2017

An alternative solution to increasing the size of the wrapper is decreasing the max-height of the mx_RoomHeader_topic by 3px.

Increasing the size of the header will only make the mis-alignment with RightPanel worse.

As mentioned in my comment, decreasing the max-height will fix the problem without impacting the mis-alignment. I've updated the PR to reflect / implement this.

Looks like I totally failed to grok what you were saying, @rxl881, sorry about that.

LGTM!

@rxl881

This comment has been minimized.

Copy link
Contributor Author

rxl881 commented Nov 9, 2017

Cool, and no worries at all @lukebarnard1. Thanks! :)

Copy link
Member

ara4n left a comment

slightly surprised at the roomheader topic changes, and column-width is new to me. but if it works, yay - looks plausible

@lukebarnard1 lukebarnard1 merged commit a0b0b6f into develop Nov 9, 2017
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@rxl881 rxl881 deleted the rxl881/roomTopic branch Nov 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.