Skip to content
This repository was archived by the owner on Sep 11, 2024. It is now read-only.

Fix scaling issues #4355

Merged
JorikSchellekens merged 33 commits into from
Apr 28, 2020
Merged

Fix scaling issues #4355

JorikSchellekens merged 33 commits into from
Apr 28, 2020

Conversation

@JorikSchellekens
Copy link
Contributor

@JorikSchellekens JorikSchellekens commented Apr 7, 2020 •
edited
Loading

This is a series small fixes for scaling which focus on the roomlist, the eventtiles and the settings window.

Things to check in review:

  • line heights
  • avatar sizes
  • vertical alignments
  • user and room pills
  • toggle alignments in preferences
  • read receipts

http://riots.im/adhoc/resize_mania2/

@JorikSchellekens JorikSchellekens requested a review from a team April 7, 2020 14:11
@JorikSchellekens JorikSchellekens force-pushed the joriks/font-scaling-fixes branch from f7dff52 to 0dac0cd Compare April 7, 2020 14:36
@turt2live
Copy link
Member

turt2live commented Apr 7, 2020

@JorikSchellekens is it possible to get gifs/screenshots of this in action?

@JorikSchellekens
Copy link
Contributor Author

JorikSchellekens commented Apr 8, 2020

Of course, I'll get some screenshots of it. You can also test it yourself by setting the font size in :root.

@JorikSchellekens
Copy link
Contributor Author

JorikSchellekens commented Apr 8, 2020

10px:
image
image
image
image

20px:
image
image
Just noticed the upload button there, I'll get that in another pr.

image

image

bwindels
bwindels reviewed Apr 10, 2020
View reviewed changes
Copy link
Contributor

@bwindels bwindels left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Have reviewed up to commit 84b8499, will continue on Tuesday.

This was a hard pill to swallow

😄

res/css/views/rooms/_BasicMessageComposer.scss Outdated
height: 16px;
width: $font-16px;
height: $font-16px;
margin-right: 0.24rem;
Copy link
Contributor

@bwindels bwindels Apr 10, 2020

Choose a reason for hiding this comment

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

Not sure where this margin comes from ... Looking at the adhoc build, the avatar also has a border around it where it didn't before:
On ad-hoc:
image
On develop:
image

Copy link
Contributor Author

@JorikSchellekens JorikSchellekens Apr 14, 2020

Choose a reason for hiding this comment

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

The previous mentions do have a border around them. I've just accidentily made them a little bigger than they should be. I'll fix it.

Copy link
Contributor Author

@JorikSchellekens JorikSchellekens Apr 14, 2020

Choose a reason for hiding this comment

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

The margin comes in as a result of replacing a large right padding used by the pill.

Copy link
Contributor Author

@JorikSchellekens JorikSchellekens Apr 14, 2020

Choose a reason for hiding this comment

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

The border is now much smaller.
image

I thought the border was intentional because of the following pills on develop:
image

I can get rid of the border on the message userpills altogether if it looks better.

Copy link
Contributor

@bwindels bwindels Apr 16, 2020

Choose a reason for hiding this comment

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

Ah, yeah, the pills do look a bit bigger in the auto-complete which I believe your screenshot was taking from?
It's a bit hard to see on your screenshot, but it looks like the padding-right is a bit smaller there, as in the text is more crammed against the end of the pill?

@bwindels
Copy link
Contributor

bwindels commented Apr 14, 2020

@JorikSchellekens what's the risk of breakage here at the default scaling factor? Just to know if we should merge this before or after releasing 1.6.

@JorikSchellekens JorikSchellekens force-pushed the joriks/font-scaling-fixes branch from 1ffe948 to fe51444 Compare April 21, 2020 14:40
@bwindels
Copy link
Contributor

bwindels commented Apr 22, 2020

@JorikSchellekens could you rebase the branch on top of current develop to make see if the e2e tests run?

@JorikSchellekens JorikSchellekens force-pushed the joriks/font-scaling-fixes branch from 309e6fc to e7111b8 Compare April 23, 2020 09:30
@JorikSchellekens
Copy link
Contributor Author

JorikSchellekens commented Apr 23, 2020

Did the rebase. The e2e test is timing out.

bwindels
bwindels approved these changes Apr 23, 2020
View reviewed changes
Copy link
Contributor

@bwindels bwindels left a comment

Choose a reason for hiding this comment

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

Lgtm, minus some minor questions.

Thanks for working on this, and sorry it took ages for me to finish the review.

@bwindels
Copy link
Contributor

bwindels commented Apr 23, 2020 •
edited
Loading

Hmm, the screenshots in the artifacts of the e2e tests do look like there might be a real problem here:
See alice and bob. You should be able to run these locally with first yarn start on riot-web and then yarn test:e2e --windowed on react-sdk.

@JorikSchellekens
Fix event tiles to smoothly resize with font.
92005b1
@JorikSchellekens
Fix pills.
e2aef3a 
This was a hard pill to swallow
@JorikSchellekens
Scale room tile heights.
334f529
@JorikSchellekens
Scale read receipt images
e034420
@JorikSchellekens
Fix avatar alignment for room state events
5113723
@JorikSchellekens
Use a function to convert to rem.
3f56ed3
@JorikSchellekens
Fix read receipt horizontal spacing at scale
ff1544b
@JorikSchellekens
Fix room header vertical alignment
b921159
@JorikSchellekens
Resize room list scroll section.
cef6868 
This fix isn't perfect. Currently the scroll view is
slightly smaller than the list of rooms. I think it has something
to do with the how the heigh is calculate in js, considering it has
some assumptions about the height of each bar and the padding. However
room items are the only things which change with respect to the root
value. Therefore the item list is actually taller than the computed
pixel value of the list converted to rems.

I'll look into it.
@JorikSchellekens
Resize toggle switches with font
f59bd53
@JorikSchellekens
Fix settings when scaling up
633c6f6
@JorikSchellekens
lint
0fe0d72
@JorikSchellekens
Room memebers avatars scale
aac90e4
@JorikSchellekens
Commuity seleciton bar vertical alignment scale fix
969f0e9
@JorikSchellekens
FIx lanugage selection alignment at scale.
997fe62
@JorikSchellekens
Fix test
18db25f
@JorikSchellekens JorikSchellekens force-pushed the joriks/font-scaling-fixes branch from b054d43 to 1f99e15 Compare April 27, 2020 16:11
@JorikSchellekens JorikSchellekens force-pushed the joriks/font-scaling-fixes branch from 1f99e15 to c995e2e Compare April 27, 2020 16:29
@JorikSchellekens JorikSchellekens merged commit cf05beb into matrix-org:develop Apr 28, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@bwindels bwindels bwindels approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@JorikSchellekens @turt2live @bwindels