-
Notifications
You must be signed in to change notification settings - Fork 219
feat(texteditor): swap editor view on touhscreens detection #5305
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
base: unstable
Are you sure you want to change the base?
feat(texteditor): swap editor view on touhscreens detection #5305
Conversation
0b938bd
to
46c5880
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.
I gave the code a good look through (I'm pretty sure I got the right commit range 😅) -- and overall it looks excellent.
I tested it locally and on touchscreen devices it was exactly as expected w/ the toolbar at the bottom & the dropdown menu. I tested on the question & hints editors successfully.
I think this is ready to go to QA
v-if="!isMobile" | ||
v-if="!isTouchDevice" |
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 see elsewhere you're using isTouchDevice || screenSizeLevel <= 3
- should this be similar or !isMobile && !isTouchDevice
?
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 condition flips the toolbar component itself (Mobile / Desktop), while the isTouchDevice || screenSizeLevel <= 3
condition manages the <AssessmentItemToolbar />
placement.
++ I've checked with Jessica and she said that the Mobile Formatting bar mustn't appear unless it's displayed on a touch device, so I manage the desktop toolbar on small screens elsewhere.
46c5880
to
6b399ae
Compare
I found a problem with the touchscreens formatting bar. Sometimes it did get positioned right over the keyboard and sometimes not. after some debugging I found out that the problem was the autoscroll behavior the browser does to keep the cursor visible. fixed in fae6155 Please try the formatting bar in browserstack to see if it aligns correctly -- @nucleogenesis |
Summary
Following a conversation with Jessica on the design thread link, In this PR I've configured the editor to switch to touch screens design based on touch device detection. in this way the design will work for tablets & mobile phones too.
for the check, I copied a utility from Kolibri here. It's not used anywhere else in studio but it'll help when we decide to prepare the whole UI of studio for touch screens too.
I also moved the
<AssessmentItemToolbar />
up on touch screens and for medium desktop screen sizes.I noticed that the overflow button in the specs had text next to the icon, I added that too.
References
currently this PR represents part of Update rich text editor to flexible, extensible rich text editing framework #5049
fixes [New Rich Text Editor]: Swap Editor UI on screen type not size #5259
Studio UI Figma Design
Reviewer Guidance
Try the editor in different screen sizes + in touch/non-touch screens
Important
This branch has a dependency on 2 PRs and should be rebased once mergedDONEVisuals
desktop.webm
RTL-Desktop.webm
Mobile.webm