[Rich text editor] Implement full screen editor mode (simple approach) #7436
Conversation
jmartinesp
requested review from
a team and
bmarty
and removed request for
a team
jmartinesp
force-pushed
the
feature/wysiwyg-full-screen-editor
branch
from
676c06e to
1d1f7b8
Compare
|
|
||
| </androidx.constraintlayout.widget.ConstraintLayout> | ||
|
|
||
| </androidx.constraintlayout.widget.ConstraintLayout> |
There was a problem hiding this comment.
It's annoying to duplicate this whole file, there is no other option? Maybe add a comment on top of the two files to warn developers about the fact that they must update the 2 files.
There was a problem hiding this comment.
We'd have to change those constraints programmatically, I used the same approach that we use for composer_text_layout.xml. I agree it's not great, but I think the alternative would be worse. I'll add the comment.
| @@ -34,6 +34,8 @@ sealed class MessageComposerAction : VectorViewModelAction { | |||
| data class SlashCommandConfirmed(val parsedCommand: ParsedCommand) : MessageComposerAction() | |||
| data class InsertUserDisplayName(val userId: String) : MessageComposerAction() | |||
|
|
|||
| object ToggleFullScreen : MessageComposerAction() | |||
There was a problem hiding this comment.
ToggleFullScreen maybe be replaced to a data class SetFullScreen(val fullscreen: Boolean), because I think in the usage there is a View to enter fullscreen and a view to exit fullscreen. Also when the message is sent, it's maybe better to use SetFullScreen(false) rather than if(fullscreen) ToggleFullScreen.
But I have not tested to implement the change, this is maybe not better, I let you choose.
There was a problem hiding this comment.
I'll try to. It's true that we have 2 sources of truth, but passing state to the MessageComposerView might not be so easy to do with no side effects.
| private val animationDuration = 100L | ||
|
|
||
| private var isFullScreen = false |
There was a problem hiding this comment.
In case of configuration change, this value (if true) may be lost.
We have 2 source of truth, this is not ideal. Can we rely only on the ViewState?
It should maybe be hidden.
|
If it's about covering the appbar, that's in the designs, but it's a bit weird to be honest. I could probably remove the scrim view elevation.
I think it's because we change its alpha value (the menu is handled again) while we animate the constraintset transition and gets animated too instead of instantly applying the values. I'll try to find a way to fix it. |
jmartinesp
force-pushed
the
feature/wysiwyg-full-screen-editor
branch
from
1d1f7b8 to
826902d
Compare
|
Sorry, there's no way to fix the blinking app icon issue without re-building this from scratch as far as I could see. We're already working on an alternative using a custom As for the shadow covering the app bar but not the statusbar, I'd have to change the way the activity works so it draws the status bar background and play with the insets... I can try to do it, but it could take some time and this is a bit urgent. I could also remove the shadow view altogether and cover the screen up to the appbar, but that wouldn't really match the designs. |
|
|
SonarCloud Quality Gate failed. |
I am talking about this banner (between my ugly red brackets):
|
#7436) * Rich text editor: implement full screen editor mode using ConstraintSets * Add back press handler * Change ToggleFullScreen to SetFullScreen, fix rebase issues * Add warning to fragment_timeline* files
Type of change
Content
A new 'maximise' button has been added to the rich text editor. This button will toggle the full screen mode of the editor, overlaying the timeline to get more room to write and focus. To achieve this we had to use a new set of constraints in
layout/fragment_timeline_fullscreen.xml, a newMessageComposerViewActionand an extra callback.Motivation and context
The full screen mode is a request from one of our clients.
Screenshots / GIFs
Tests
Tested devices
Checklist