Fix: truncate pills if they are too long #7455

nimau · 2023-03-28T10:17:58Z

This PR fixes #7413

Pull Request Checklist

I read the contributing guide
UI change has been tested on both light and dark themes, in portrait and landscape orientations and on iPhone and iPad simulators
Accessibility has been taken into account.

Pull request is based on the develop branch

Pull request contains a changelog file in ./changelog.d
You've made a self review of your PR
Pull request includes screenshots or videos of UI changes
Pull request includes a sign off

codecov · 2023-03-28T10:57:35Z

Codecov Report

Patch coverage: 35.29% and no project coverage change.

Comparison is base (6e48692) 12.30% compared to head (9776233) 12.30%.

❗ Current head 9776233 differs from pull request most recent head 334a116. Consider uploading reports for the commit 334a116 to get more accurate results

Additional details and impacted files

@@           Coverage Diff            @@
##           develop    #7455   +/-   ##
========================================
  Coverage    12.30%   12.30%           
========================================
  Files         1645     1645           
  Lines       162982   162992   +10     
  Branches     66914    66916    +2     
========================================
+ Hits         20048    20056    +8     
- Misses      142285   142287    +2     
  Partials       649      649

Flag	Coverage Δ
uitests	`55.25% <ø> (+0.01%)`	⬆️
unittests	`6.19% <35.29%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
Riot/Modules/Pills/PillAttachmentView.swift	`4.87% <0.00%> (-0.04%)`	⬇️
...iot/Modules/Pills/PillAttachmentViewProvider.swift	`0.00% <0.00%> (ø)`
Riot/Modules/Pills/PillTextAttachment.swift	`68.47% <100.00%> (+0.70%)`	⬆️
Riot/Modules/Pills/PillTextAttachmentData.swift	`87.83% <100.00%> (+0.51%)`	⬆️

... and 1 file with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

pixlwave

LGTM 👍

pixlwave · 2023-03-29T14:12:21Z

Riot/Modules/Pills/PillTextAttachmentData.swift

@@ -108,6 +112,7 @@ struct PillTextAttachmentData: Codable {
        case isHighlighted
        case alpha
        case font
+        case maxWidth


My only question is about encoding/decoding these pills - I'm not sure where/when we do it.

Does it make sense to encode a computed maxWidth with them? Could it be decoded to be displayed in a different size window (e.g. If the user is now a different split size on iPad)? If so, maybe it would make sense to not encode it, and instead set it back to .greatestFiniteMagnitude?

Thanks @pixlwave, I've updated the code to reflect your suggestions.

pixlwave · 2023-03-29T14:13:00Z

Riot/Modules/Pills/PillTextAttachmentData.swift

@@ -93,12 +95,14 @@ struct PillTextAttachmentData: Codable {
         items: [PillTextAttachmentItem],
         isHighlighted: Bool,
         alpha: CGFloat,
-         font: UIFont) {
+         font: UIFont,
+         maxWidth: CGFloat = CGFloat.greatestFiniteMagnitude) {


Should be able to infer the type of the default here :)

Suggested change

maxWidth: CGFloat = CGFloat.greatestFiniteMagnitude) {

maxWidth: CGFloat = .greatestFiniteMagnitude) {