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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add width to roomTile_nameStack in _InviteDialog.scss #8092

Closed
wants to merge 1 commit into from

Conversation

SpookyDooky
Copy link

@SpookyDooky SpookyDooky commented Mar 20, 2022

This PR fixes vector-im/element-web#19463 by adding a width to roomTile_namestack in _InviteDialgo.scss.

Before:
image

After:
image

Please add the T-Defect label.

Signed-off-by: Raymond d'Anjou r.danjou@student.tudelft.nl


Here's what your changelog entry will look like:

馃悰 Bug Fixes

  • Add width to roomTile_nameStack in _InviteDialog.scss (#8092). Contributed by @SpookyDooky.

Preview: https://pr8092--matrix-react-sdk.netlify.app
鈿狅笍 Do you trust the author of this PR? Maybe this build will steal your keys or give you malware. Exercise caution. Use test accounts.

@SpookyDooky SpookyDooky requested a review from a team as a code owner March 20, 2022 13:15
@turt2live turt2live added the T-Defect Bugs, crashes, hangs, vulnerabilities, or other reported problems label Mar 22, 2022
@@ -195,6 +195,7 @@ limitations under the License.
.mx_InviteDialog_roomTile_nameStack {
display: inline-block;
overflow: hidden;
width: calc(70%);
Copy link
Member

Choose a reason for hiding this comment

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

why 70%?

Copy link
Author

Choose a reason for hiding this comment

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

It was chosen so that the layout remained intact, because if it was set to 100% the name would appear under the users avatar like in the first picture, it would also push the "23 hours ago" underneath the box. Setting it to 70% fixed those issues.

Copy link
Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@luixxiul luixxiul Mar 26, 2022

Choose a reason for hiding this comment

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

By setting so, the time strings can be placed under the nameStack due to float and/or text-align stuff.

before

It looks like the display style of roomTile in which they are placed needs changing with flex/grid. From my trial I guess that should be the most straightforward way.

temp

I am not sure if ellipsis functionality should be enabled here from the perspective of UX, though.

Copy link
Member

Choose a reason for hiding this comment

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

The issue with 70% is that the UI cuts off prematurely in the screenshots, so I suspect the change is a bit more involved than what is implied here.

Also, magic numbers like these need comments in the code.

@codecov
Copy link

codecov bot commented Mar 22, 2022

Codecov Report

Merging #8092 (e702289) into develop (d7a3f39) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           develop    #8092   +/-   ##
========================================
  Coverage    26.66%   26.66%           
========================================
  Files          870      870           
  Lines        52166    52166           
  Branches     13217    13217           
========================================
  Hits         13910    13910           
  Misses       38256    38256           

@turt2live turt2live self-requested a review March 25, 2022 15:16
@turt2live turt2live removed their request for review April 4, 2022 06:38
@kittykat
Copy link
Contributor

kittykat commented May 4, 2022

@SpookyDooky I wanted to check in if you're intending to update your PR with the requested changes? If not, we'd like to unassign the issue in case anyone else would like to pick it up 馃憤

@MadLittleMods MadLittleMods added the Z-Community-PR Issue is solved by a community member's PR label Jun 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-Defect Bugs, crashes, hangs, vulnerabilities, or other reported problems Z-Community-PR Issue is solved by a community member's PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ellipses functionality missing in quite a few places which can completly break the UI
5 participants