-
Notifications
You must be signed in to change notification settings - Fork 221
feat(texteditor): Mobile view and responsiveness enhancements #5220
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
feat(texteditor): Mobile view and responsiveness enhancements #5220
Conversation
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.
Excellent work overall @habibayman! The overflow works very well and after giving things a thorough look I left a few comments that are mostly style notes.
I think that if the bug in the image below can be resolved (I think most likely be tying the conditional mobile-vs-desktop toolbar logic to the editor width rather than screen width) -- then this PR will be all set.

// TODO: Maybe these shouldnt be hardcoded? | ||
const OVERFLOW_BREAKPOINTS = { | ||
insert: 750, | ||
script: 650, | ||
lists: 550, | ||
clearFormat: 450, | ||
clipboard: 395, | ||
}; |
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.
This being hardcoded actually seems like a good way to go - although maybe it could be made more clear that this is referencing the width of the toolbar as opposed to the screen width.
The primary downside of this hardcoding that comes to mind is that if we add/remove buttons to any category, these numbers will all have to be manually recalculated and updated.
I think we should certainly test this out a bit across devices and see if it works as expected for now and we could make a follow-up issue to dynamically calculate the categories' widths themselves or something. I'm not sure how likely we are to add/remove actions in the future (thoughts @rtibbles @marcellamaki).
It could be as straightforward as getting a ref to the categories' topmost containers and calculating their width maybe?
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.
That said, I saw the dropdown arrow go just out of bounds before clipboard
was folded into the overflow menu but putting it at 410 or so made it look smooth.
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.
I saw the dropdown arrow go just out of bounds
💯 I set it to 405 and it's perfectly fine now.
THIS IS one of the things that are making me hesitant about the way the breakpoints exist, they're updated based on trial and error and small calculations.
Plus of course how it breaks everything about the open/closed principle if we decided to update any of the buttons too.
It works now so I guess we can consider this as non-blocking but I will open a separate issue for that so we can get back to.
<div class="editor-container"> | ||
<EditorToolbar | ||
@insert-image="target => openCreateModal({ targetElement: target })" | ||
v-if="!isMobile" | ||
@insert-image="target => imageHandler.openCreateModal({ targetElement: target })" | ||
@insert-link="linkHandler.openLinkEditor()" | ||
@insert-math="target => mathHandler.openCreateMathModal({ targetElement: target })" | ||
/> | ||
|
||
<div v-else> | ||
<MobileTopBar | ||
@insert-image="target => imageHandler.openCreateModal({ targetElement: target })" | ||
@insert-link="linkHandler.openLinkEditor()" | ||
@insert-math="target => mathHandler.openCreateMathModal({ targetElement: target })" | ||
/> | ||
<MobileFormattingBar | ||
v-if="isFocused" | ||
@insert-image="target => imageHandler.openCreateModal({ targetElement: target })" | ||
@insert-link="linkHandler.openLinkEditor()" | ||
@insert-math="target => mathHandler.openCreateMathModal({ targetElement: target })" | ||
/> | ||
</div> |
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.
(non-blocking style suggestion) Since the event handlers and their values are all the same whether you're using EditorToolbar
or MobileTopBar
you could use the component
tag like so:
<div class="editor-container">
<component
:is="isMobile ? 'MobileTopBar' : 'EditorToolbar'"
@insert-image="..."
@insert-link="..."
@insert-math="..."
/>
<MobileFormattingBar
v-if="isFocused && isMobile"
...
/>
</div>
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.
This is annoying when the values for props/event handlers would differ depending on the component type but a convenience when they line up
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.
THANK YOU for bringing this up!!, this caught my eyes once too and I wanted to refactor it seems like I got carried away with another thing.
The problem is that we have 3 components sharing the same handlers not just 2, so the dynamic component approach will not get us the most cleanup here I think?
I did this instead which IMO is fine.
<EditorToolbar
v-if="!isMobile"
v-on="sharedEventHandlers"
/>
<div v-else>
<MobileTopBar v-on="sharedEventHandlers" />
<MobileFormattingBar
v-if="isFocused"
v-on="sharedEventHandlers"
/>
</div>
...entcuration/contentcuration/frontend/shared/views/TipTapEditor/TipTapEditor/TipTapEditor.vue
Show resolved
Hide resolved
...contentcuration/frontend/shared/views/TipTapEditor/TipTapEditor/components/EditorToolbar.vue
Outdated
Show resolved
Hide resolved
...entcuration/contentcuration/frontend/shared/views/TipTapEditor/TipTapEditor/TipTapEditor.vue
Show resolved
Hide resolved
3bd473d
to
ef1ad2f
Compare
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.
All of Jacob's feedback has been addressed
Summary
This PR includes an implementation for the mobile view, in addition to enhancing the responsiveness of the design in general.
It also includes some refactoring because the main changes that the PR aimed to include, exposed some areas where refactoring became the right choice.
Since the mobile view is different from the desktop design to a point where it can't just be fixed with just CSS tweaks or media queries. I did some thinking&research and decided to take a Conditional Toolbar Layout approach where I've created different components for different screen sizes.
Important
This branch has a dependency onDONEPR #5189
commit 1 replace with new editor - commit 2 comment old editor's tests
Visuals
demo-desktop-overflow.webm
(Video recorded after replacing the editor in studio)
Mobile-View.webm
References
Figma Design
currently this PR represents part of Update rich text editor to flexible, extensible rich text editing framework #5049
fixes [New Rich Text Editor]: Touch screens & Design responsiveness #5184
Reviewer guidance
The changes made are all about how the editor will "fit" wrt other page elements/sizes;
SO it isn't any helpful that we test this in the isolated dev route.
For this I've temporarily replaced the old edior with the new one in
AnswrsEditor.vue
.To be able to test the changes: