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

[Rich text editor] Add link functionality to rich text editor #1309

Merged
merged 23 commits into from
Sep 19, 2023

Conversation

jonnyandrew
Copy link
Contributor

@jonnyandrew jonnyandrew commented Sep 13, 2023

Type of change

  • Feature
  • Bugfix
  • Technical
  • Other :

Content

  • Add link functionality to rich text editor
  • Add 'list dialog' component compound design library
  • Add 'text field list item' component to compound design library

Motivation and context

Screenshots / GIFs

See screenshot tests

Tests

  • Enable rich text editor in developer options
  • Start composing a message
  • Add a link using the new button
  • Edit the link using the new button
  • Remove the link using the new button
  • Send a message containing a link
  • Edit a message containing a link

Tested devices

  • Physical
  • Emulator
  • OS version(s): 13

Checklist

@github-actions
Copy link
Contributor

github-actions bot commented Sep 13, 2023

📱 Scan the QR code below to install the build (arm64 only) for this PR.
QR code
If you can't scan the QR code you can install the build via this link: https://i.diawi.com/d9uo9m

@codecov
Copy link

codecov bot commented Sep 13, 2023

Codecov Report

Patch coverage: 44.23% and project coverage change: -0.07% ⚠️

Comparison is base (1eb9491) 57.81% compared to head (e26f76a) 57.74%.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1309      +/-   ##
===========================================
- Coverage    57.81%   57.74%   -0.07%     
===========================================
  Files         1120     1123       +3     
  Lines        29774    29970     +196     
  Branches      6114     6174      +60     
===========================================
+ Hits         17213    17307      +94     
- Misses        9876     9940      +64     
- Partials      2685     2723      +38     
Files Changed Coverage Δ
...ent/android/libraries/textcomposer/TextComposer.kt 53.15% <3.84%> (-1.77%) ⬇️
...ries/designsystem/components/dialogs/ListDialog.kt 44.68% <44.68%> (ø)
...d/libraries/textcomposer/TextComposerLinkDialog.kt 50.00% <50.00%> (ø)
.../designsystem/components/list/TextFieldListItem.kt 66.66% <66.66%> (ø)

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jonnyandrew jonnyandrew marked this pull request as ready for review September 14, 2023 08:18
@jonnyandrew jonnyandrew requested a review from a team as a code owner September 14, 2023 08:18
@jonnyandrew jonnyandrew requested review from jmartinesp and removed request for a team September 14, 2023 08:18
@jmartinesp
Copy link
Member

Should the link dialog pre-set the selected text (if any) as the text for the link? It doesn't seem to be working.

Copy link
Member

@jmartinesp jmartinesp left a comment

Choose a reason for hiding this comment

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

Besides the possible issue with inserting a link not taking the selected text, I have a few questions and suggestions.

@ElementBot
Copy link
Collaborator

ElementBot commented Sep 14, 2023

Warnings
⚠️

gradle/libs.versions.toml#L99 - A newer version of androidx.compose.material3:material3 than 1.2.0-alpha06 is available: 1.2.0-alpha07

⚠️

libraries/ui-strings/src/main/res/values-cs/translations.xml#L160 - For locale "cs" (Czech) the following quantity should also be defined: many (e.g. "10.0 dne")

⚠️

libraries/ui-strings/src/main/res/values-cs/translations.xml#L165 - For locale "cs" (Czech) the following quantity should also be defined: many (e.g. "10.0 dne")

⚠️

libraries/ui-strings/src/main/res/values-sk/translations.xml#L173 - For locale "sk" (Slovak) the following quantity should also be defined: many (e.g. "10.0 dňa")

⚠️

libraries/ui-strings/src/main/res/values-sk/translations.xml#L178 - For locale "sk" (Slovak) the following quantity should also be defined: many (e.g. "10.0 dňa")

Generated by 🚫 dangerJS against e26f76a

@jonnyandrew jonnyandrew mentioned this pull request Sep 14, 2023
1 task
@jonnyandrew
Copy link
Contributor Author

Should the link dialog pre-set the selected text (if any) as the text for the link? It doesn't seem to be working.

The rich text editor is still limited in that respect and it's not possible to preview or edit the selected text when editing links.

However, in the version you tested there is a bug where the dialog may incorrectly display an empty text field when selecting text; this makes the problem much worse UX wise and I suspect this is what you saw. I've since updated to version 2.10.2 which contains a fix for this if you wanted to try this.

tools/localazy/config.json Outdated Show resolved Hide resolved
@bmarty bmarty mentioned this pull request Sep 14, 2023
1 task
@sonarcloud
Copy link

sonarcloud bot commented Sep 15, 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
0.0% 0.0% Duplication

@jonnyandrew
Copy link
Contributor Author

@jmartinesp / @bmarty thanks for your reviews - I have addressed the issues

Copy link
Member

@bmarty bmarty left a comment

Choose a reason for hiding this comment

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

Tested OK, thanks. There are conflict on the strings (sorry), maybe run again the localazy script?

@jonnyandrew jonnyandrew merged commit ee8d27e into develop Sep 19, 2023
11 of 13 checks passed
@jonnyandrew jonnyandrew deleted the jonny/rte-links branch September 19, 2023 11:20
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.

6 participants