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

Turning permalinks into pills #7432

Merged
merged 10 commits into from
Mar 21, 2023
Merged

Turning permalinks into pills #7432

merged 10 commits into from
Mar 21, 2023

Conversation

nimau
Copy link
Contributor

@nimau nimau commented Mar 17, 2023

This PR fixes the following issues:
#7409: Permalinks to a room/space are pillified
#7411: Permalinks to a matrix user are pillified
#7412: Permalinks to messages are pillified

#7411: Permalinks to a matrix user are pillified

Here is an overview of the result for user mentions:
pills-user-before

It now looks like this:

pills-user-after-edited

#7409: Permalinks to a room/space are pillified

Here is an overview of the result for room mentions:
pills-room-before

It now looks like this:

pills-room-after

#7412: Permalinks to messages are pillified

Here is an overview of the result for message mentions:
pills-message-before

It now looks like this:

pills-message-after

#7409: Permalinks to a room/space are pillified
#7411: Permalinks to a matrix user are pillified
#7412: Permalinks to messages are pillified
@nimau nimau marked this pull request as ready for review March 17, 2023 16:43
@nimau nimau requested a review from aringenbach March 17, 2023 16:43
@gaelledel
Copy link

@nimau All good from a design perspective - please do not change the style of the pill written in the current implementation. We will review this as part of elementX reskin instead

@codecov
Copy link

codecov bot commented Mar 20, 2023

Codecov Report

Patch coverage has no change and project coverage change: -0.04 ⚠️

Comparison is base (31de127) 12.13% compared to head (ed954cb) 12.10%.

❗ Current head ed954cb differs from pull request most recent head 83a10e2. Consider uploading reports for the commit 83a10e2 to get more accurate results

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #7432      +/-   ##
===========================================
- Coverage    12.13%   12.10%   -0.04%     
===========================================
  Files         1643     1643              
  Lines       162532   162427     -105     
  Branches     66735    66711      -24     
===========================================
- Hits         19730    19664      -66     
+ Misses      142157   142120      -37     
+ Partials       645      643       -2     
Flag Coverage Δ
uitests 55.23% <ø> (ø)

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

Impacted Files Coverage Δ
...MatrixKit/Utils/EventFormatter/MXKEventFormatter.m 17.01% <ø> (+0.01%) ⬆️
Riot/Modules/MatrixKit/Utils/MXKTools.m 22.18% <ø> (-0.10%) ⬇️
Riot/Modules/Pills/PillAttachmentView.swift 28.57% <ø> (+7.88%) ⬆️
...iot/Modules/Pills/PillAttachmentViewProvider.swift 0.00% <ø> (-25.65%) ⬇️
Riot/Modules/Pills/PillTextAttachment.swift 69.44% <ø> (-3.06%) ⬇️
Riot/Modules/Pills/PillTextAttachmentData.swift 92.00% <ø> (+2.00%) ⬆️
Riot/Modules/Pills/PillsFormatter.swift 80.00% <ø> (-3.34%) ⬇️
Riot/Modules/Spaces/Avatar/SpaceAvatarView.swift 0.00% <ø> (ø)
Riot/Modules/User/Avatar/UserAvatarView.swift 80.00% <ø> (ø)

... 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.

Copy link
Contributor

@aringenbach aringenbach left a comment

Choose a reason for hiding this comment

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

Looks pretty good !

A few comments on minor things

Also noticed the following behaviours, but these aren't new bugs it seems (but they are probably not tracked), I think we can create issues for these (if you agree):

  • The links displayed for unknown Room coming from a Markdown share a global issue with Element-iOS Markdown links: if we use edit on them we will display actual links in the legacy/plain text composer, but these get translated to plain text when sending the edited messages (this isn't an issue from this change we can easily reproduce on develop with Markdown alone)
  • Our Copy functionality pulls out markdown, which is rather convenient for bringing a message outside of Element-iOS, but if we paste internally in the composer, we will display the Markdown for a user/room instead (this will still be converted to a Pill in the timeline if we have data for that user, but behaviour is not optimal since the Composer is able to display Pills)

Riot/Modules/Pills/PillTextAttachmentData.swift Outdated Show resolved Hide resolved
Riot/Modules/Pills/PillTextAttachmentData.swift Outdated Show resolved Hide resolved
Riot/Modules/Pills/PillAttachmentView.swift Outdated Show resolved Hide resolved
Riot/Modules/Pills/PillAttachmentView.swift Outdated Show resolved Hide resolved
RiotTests/PillsFormatterTests.swift Outdated Show resolved Hide resolved
RiotTests/PillsFormatterTests.swift Outdated Show resolved Hide resolved
@nimau
Copy link
Contributor Author

nimau commented Mar 21, 2023

@aringenbach I have made the requested changes as well as some corrections for the issues I found. Can you please have a look ?

@nimau nimau requested a review from aringenbach March 21, 2023 10:24
Copy link
Contributor

@aringenbach aringenbach left a comment

Choose a reason for hiding this comment

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

Can you take a look at this failing test: - (void)testLinkWithRoomAliasLink ? Seems related to this change.

Otherwise LGTM

Riot/Modules/Pills/PillsFormatter.swift Outdated Show resolved Hide resolved
@sonarcloud
Copy link

sonarcloud bot commented Mar 21, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@nimau nimau merged commit a660e3e into develop Mar 21, 2023
@nimau nimau deleted the nimau/PSB-59-pills branch March 21, 2023 13:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants