-
-
Notifications
You must be signed in to change notification settings - Fork 3.3k
#6076 - Fix "Loading more messages..." not readable on custom chat wa… #6084
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
base: main
Are you sure you want to change the base?
Conversation
Hey @kyvg, have you tested this change in both light and dark mode? Can you post pictures? |
Hey @MarlowBrown! Yes, I’ve tested the change in both light and dark mode. Here are the screenshots: But I'm not sure about the design though. Maybe it would better to use a UIActivityIndicatorView instead of the blurred label? It’s not very high-contrast, but thanks to the animation, it's still visible against the any background. |
Hello again @kyvg The blurred label definitely looks better, and it's not too much of a change! If you were to use an Activity Indicator I think that would have a higher chance of needing to pass through the UI engineer on the team. Also, have you tested changing the text font size through accessibility to make sure that the blur view stretches appropriately? |
65f2430
to
98c13fc
Compare
Hi @MarlowBrown. The font size is fixed, same for the header view. So it doesn't scale with larger accessibility font sizes. I've just noticed that longer labels on smaller screens weren't truncating as before, so I pinned the blurView to the superview’s leading and trailing edges. Now it works correctly |
Sounds good, @kyvg! The only thing left is to ensure that everything looks good on the iPad. |
@MarlowBrown, sure, here is screenshots from the iPad |
This looks good! Generally, the developers maintain a private repository where they implement modifications, which are then pushed to our current repository. They will incorporate your commit, close your ticket, and subsequently add it to their private repository. The commit will then be included in a release within a couple of weeks. To process the commit, the developers will require an email address to associate with your commit. If you do not already have a valid email address linked to your commit, the developers will contact you to obtain one. Typically, @sashaweiss-signal reviews the repository, although I have also seen @max-signal active here. Regardless, I appreciate your efforts in making this change! |
…m chat wallpaper
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your interest. The adding of a blur view seems fine to me, but the layout here has changed in ways I don't think are correct.
I'd also be interested in what this looks like when UIAccessibility.isReduceTransparencyEnabled
is enabled (iOS: Settings → Accessibility → Display & Text Size → Reduce Transparency).
addSubview(label) | ||
label.autoPinEdgesToSuperviewEdges() | ||
label.autoSetDimension(.height, toSize: LoadMoreMessagesView.fixedHeight) | ||
addSubview(blurView) | ||
blurView.contentView.addSubview(label) | ||
|
||
blurView.autoPinEdge(toSuperviewEdge: .leading, withInset: 16, relation: .greaterThanOrEqual) | ||
blurView.autoPinEdge(toSuperviewEdge: .trailing, withInset: 16, relation: .greaterThanOrEqual) | ||
|
||
label.autoPinEdgesToSuperviewEdges(with: UIEdgeInsets(top: 8, left: 16, bottom: 8, right: 16)) | ||
label.textAlignment = .center | ||
blurView.autoCenterInSuperview() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding a blur seems reasonable, but the layout has changed here. I believe the correct thing would be to lay out the blur view the way that label
is laid out today:
- Pin blur view to superview edges
- Set height of blur view to `LoadMoreMessagesView.fixedHeight
And then make label
a subview and pin it to the superview edges with an inset.
To avoid making the label smaller, due to the inset, I'd suggest increasing fixedHeight
by 16, to compensate for the 8pt of inset on the top and bottom.
As it is, the height of this is now indeterminate, which could cause subtle layout issues because other components reference fixedHeight
.
Checking in on this ticket. @kyvg Are you still working on this? |
First time contributor checklist
Contributor checklist
Description
The pull request fixes #6076 by adding a blurred background to "Loading more messages..." label
