-
Notifications
You must be signed in to change notification settings - Fork 500
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
Conversation
19fb372
to
64ea190
Compare
@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 ReportPatch coverage has no change and project coverage change:
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
Flags with carried forward coverage won't be shown. Click here to find out more.
... 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. |
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.
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)
@aringenbach I have made the requested changes as well as some corrections for the issues I found. Can you please have a look ? |
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.
Can you take a look at this failing test: - (void)testLinkWithRoomAliasLink
? Seems related to this change.
Otherwise LGTM
Kudos, SonarCloud Quality Gate passed! |
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:
It now looks like this:
#7409: Permalinks to a room/space are pillified
Here is an overview of the result for room mentions:
It now looks like this:
#7412: Permalinks to messages are pillified
Here is an overview of the result for message mentions:
It now looks like this: