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.

Sign up for GitHub

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

Jump to bottom

[Rich text editor] Add support for links in the rich text editor #7746

Merged
merged 1 commit into from Dec 21, 2022
Merged

[Rich text editor] Add support for links in the rich text editor #7746

merged 1 commit into from Dec 21, 2022

Conversation

jonnyandrew
Copy link
Contributor

@jonnyandrew jonnyandrew commented Dec 8, 2022 •
edited

Type of change

  • Feature
  • Bugfix
  • Technical
  • Other :

Content

Add the ability to create and edit links using the rich text editor.

Motivation and context

Screenshots / GIFs

links.webm

Tests

  • Enable the rich text editor via Labs
  • Start typing a message
  • Highlight/select some text in the editor
    • Tap the new link icon
    • Set and save the link
  • Ensure no text is highlighted/selected
    • Set text and and save the link
  • Tap in a space
    • Set text and and save the link
  • Select some linked text
    • Edit the link and save
  • Select some linked text
    • Remove the link

Tested devices

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

Checklist

ElementBot
ElementBot reviewed Dec 8, 2022
View reviewed changes
...or/src/main/java/im/vector/app/features/home/room/detail/composer/MessageComposerFragment.kt Outdated Show resolved Hide resolved
...or/src/main/java/im/vector/app/features/home/room/detail/composer/MessageComposerFragment.kt Outdated Show resolved Hide resolved
vector/src/main/java/im/vector/app/features/home/room/detail/composer/RichTextComposerLayout.kt Outdated Show resolved Hide resolved
vector/src/main/java/im/vector/app/features/home/room/detail/composer/link/SetLinkAction.kt Outdated Show resolved Hide resolved
vector/src/main/java/im/vector/app/features/home/room/detail/composer/link/SetLinkAction.kt Outdated Show resolved Hide resolved
vector/src/main/java/im/vector/app/features/home/room/detail/composer/link/SetLinkViewEvents.kt Outdated Show resolved Hide resolved
vector/src/main/java/im/vector/app/features/home/room/detail/composer/link/SetLinkViewEvents.kt Outdated Show resolved Hide resolved
vector/src/main/java/im/vector/app/features/home/room/detail/composer/link/SetLinkViewModel.kt Show resolved Hide resolved
vector/src/main/java/im/vector/app/features/home/room/detail/composer/link/SetLinkViewModel.kt Show resolved Hide resolved
.../src/test/java/im/vector/app/features/home/room/detail/composer/link/SetLinkViewModelTest.kt Outdated Show resolved Hide resolved
@jonnyandrew jonnyandrew force-pushed the feat/PSU-1023-links branch 2 times, most recently from 65c30f4 to 994dddf Compare December 8, 2022 17:14
@sonarcloud
Copy link

sonarcloud bot commented Dec 9, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

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

37.7% 37.7% Coverage
0.0% 0.0% Duplication

@jonnyandrew jonnyandrew marked this pull request as ready for review December 9, 2022 12:10
@jonnyandrew jonnyandrew requested review from a team, ganfra and callumu and removed request for a team December 9, 2022 12:10
ganfra
ganfra reviewed Dec 15, 2022
View reviewed changes
Copy link
Contributor

@ganfra ganfra left a comment

Choose a reason for hiding this comment

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

One remark otherwise LGTM

vector/src/main/java/im/vector/app/core/platform/VectorBaseDialogFragment.kt
/**
* Add Mavericks capabilities, handle DI and bindings.
*/
abstract class VectorBaseDialogFragment<VB : ViewBinding> : DialogFragment(), MavericksView {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why creating this new baseclass ? the ui doesn't look like a Dialog. Also we use BottomSheet usually? It's weird

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason I didn't use a bottom sheet is because the design is supposed to look like a 'full screen dialog' (see this in the Material spec). I didn't want to fight against the bottom sheet dialog implementation to achieve this full screen design.

Similar features' UI (e.g. polls) use a new Activity to achieve this design but I didn't want to introduce a whole new Activity for this simple modal interaction. I also wanted to be able to communicate to the calling Fragment using a ViewModel which is not possible from a separate Activity.

Hope that makes sense?

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it, lets keep it like this then, thanks!

@jonnyandrew jonnyandrew merged commit 5046679 into develop Dec 21, 2022
@jonnyandrew jonnyandrew deleted the feat/PSU-1023-links branch December 21, 2022 10:40
@ghost
Copy link

ghost commented Dec 30, 2022

I've been doing some testing on my Pixel 6a and it's working good so far, but I wanted to note that this feature will probably make this issue more discoverable by users.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ElementBot ElementBot ElementBot left review comments

@ganfra ganfra ganfra approved these changes

@callumu callumu Awaiting requested review from callumu

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@jonnyandrew @ganfra @ElementBot