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

Apply design changes to WYSIWYG editor #7354

Merged
merged 6 commits into from
Oct 14, 2022

Conversation

jmartinesp
Copy link
Member

@jmartinesp jmartinesp commented Oct 13, 2022

Type of change

  • Feature
  • Bugfix
  • Technical
  • Other : Apply the right designs to the rich text editor.

Content

Adjusted sizes, round corners, colours, backgrounds, added menu state feedback, fixed proguard issues.

Motivation and context

#7288 only set up the initial integration, but we still needed to implement some of the design decisions.

Screenshots / GIFs

Before After
Screenshot_1665654635 Screenshot_1665654454

Tests

  • Enable rich text editor in lab flags.
  • Open a DM/room and use formatting.

Tested devices

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

Checklist

@jmartinesp jmartinesp requested a review from a team October 13, 2022 09:54
@jmartinesp jmartinesp self-assigned this Oct 13, 2022
@jmartinesp jmartinesp requested review from onurays and removed request for a team October 13, 2022 09:54
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.

Some small remarks. Also the CI is not happy because of the usage of colorAccent. Can you check please? (this is maybe a false positive)

<style name="Widget.Vector.EditText.Composer.RichText" parent="Widget.AppCompat.EditText">
Copy link
Member

Choose a reason for hiding this comment

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

I would rename to Widget.Vector.EditText.RichTextComposer, to avoid the confusion with the parenting (parent style is Widget.AppCompat.EditText, not Widget.Vector.EditText.Composer).

Or eventually just write:

    <style name="Widget.Vector.EditText.Composer.RichText">
        <item name="android:minHeight">20dp</item>
        <item name="android:padding">0dp</item>
    </style>

android:background="@android:color/transparent"
android:contentDescription="@string/app_name">
android:background="@drawable/bg_rich_text_menu_button"
android:contentDescription="@string/rich_text_editor_format_bold"
Copy link
Member

Choose a reason for hiding this comment

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

Better to use tools:contentDescription, since this will be changed at runtime.

Copy link
Member Author

Choose a reason for hiding this comment

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

It seems like this is not enough. See #7354 (comment).

@jmartinesp jmartinesp force-pushed the misc/apply-designs-to-wysiwyg-editor branch from dce40c9 to b07f6f5 Compare October 13, 2022 13:15
@jmartinesp jmartinesp force-pushed the misc/apply-designs-to-wysiwyg-editor branch from b07f6f5 to 743520a Compare October 13, 2022 14:16
<style name="Widget.Vector.EditText.Composer.RichTextComposer" parent="Widget.AppCompat.EditText">
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<style name="Widget.Vector.EditText.Composer.RichTextComposer" parent="Widget.AppCompat.EditText">
<style name="Widget.Vector.EditText.RichTextComposer" parent="Widget.AppCompat.EditText">

?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, I changed it to the correct value, and also corrected the references to this style in the views.

@jmartinesp jmartinesp requested a review from bmarty October 13, 2022 15:48
@jmartinesp
Copy link
Member Author

There are a couple of other design changes to review.

@jmartinesp jmartinesp force-pushed the misc/apply-designs-to-wysiwyg-editor branch from 73e9d03 to a38255b Compare October 13, 2022 16:18
@ElementBot
Copy link

Warnings
⚠️

vector/src/main/java/im/vector/app/features/home/room/detail/composer/voice/VoiceMessageViews.kt#L63 - Custom view AudioWaveformView has setOnTouchListener called on it but does not override performClick

⚠️

vector/src/main/java/im/vector/app/features/home/room/detail/composer/voice/VoiceMessageViews.kt#L63 - Custom view AudioWaveformView has setOnTouchListener called on it but does not override performClick

⚠️

vector/src/main/java/im/vector/app/features/home/room/detail/composer/voice/VoiceMessageViews.kt#L63 - onTouch lambda should call View#performClick when a click is detected

⚠️

vector/src/main/java/im/vector/app/features/home/room/detail/composer/voice/VoiceMessageViews.kt#L63 - onTouch lambda should call View#performClick when a click is detected

Generated by 🚫 dangerJS against a38255b

@sonarqubecloud
Copy link

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

0.0% 0.0% Coverage
0.0% 0.0% Duplication

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.

Thanks for the update. Latest commits are fine too. 👍

@jmartinesp jmartinesp merged commit 81ef141 into develop Oct 14, 2022
@jmartinesp jmartinesp deleted the misc/apply-designs-to-wysiwyg-editor branch October 14, 2022 07:59
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