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

Very good and cute attempt to fix formatting toolbar on iOS. Positivity #39

Merged
merged 8 commits into from
Jun 10, 2019

Conversation

lowercasename
Copy link
Collaborator

Okay so I've played around a bit with medium-editor's options. On iOS Firefox, neither adjusting the floating toolbar's CSS to display it below text, nor setting it to appear in a container div below the contenteditable div with relativeContainer, makes the toolbar come up at all, which is dang annoying. Not sure what the issue is there. Of those options, the latter is the most sensible, I think.

In this PR I've enabled the third, least good option, which is to set the toolbar to static on mobile display sizes and below and adjust the padding of the contenteditable div to allow the iOS default toolbar to sit below the formatting toolbar. In that PR I've also got the necessary CSS to make it appear below text if it isn't set to static, commented out.

@toBeOfUse
Copy link
Contributor

toBeOfUse commented May 19, 2019

Yeah, this editor is really not built for mobile. All of the issues on their Github raising this point just sort of trail off, as I guess you said you've seen. Per the linked discussions (and specifically this rather uninspiring comment), the reason why the toolbar isn't showing up for you on iOS is that the editor listens for a mouseup event to find out that the user has just finished making a selection. (I was bringing it up in the screenshot in issue #37 by double-tapping, which is a selection action on Android that works identically to double-click to select on desktop.) To change that we'd have to download the un-minified version of the script to our server and start changing it around to support touch events ourselves.

So that's the main argument for making the toolbar static on mobile. If we do that, my vote is for putting it below the textbox, as that doesn't necessitate throwing in 30 pixels of padding at the top of the box for what will appear to the user at first glance to be there for absolutely no reason at all.

There is another option. If the code that you commented out with the relative container thing was the version you were trying to work with, you needed to use getElementsByClassName(...)[0], the selector that you'd put in there matched the element's class, not its id. I did get the toolbar to attach to that element, which in theory means we can position that element absolutely and make the toolbar hover to wherever we want, but again, the toolbar won't materialize without a mouseup event unless we add code to the original MediumEditor file to make it listen for touch events. That probably wouldn't be hard? This guy claims he accomplished it in one line, which, maybe. So, do that, and then write the positioning code to try to get the toolbar to be show up below the thing you've selected every time? ...Which is code that you're saying you have, although I don't know where it is presently, I'll be damned if I can figure out how that CSS that was commented out at the bottom of styles.css does anything other than move the little pointing MediumEditor arrow? So. That is the other option.

@toBeOfUse
Copy link
Contributor

(Ignore my commits, I think/hope they're cancelling each other out.) Not bad, not bad. I'd missed that thing with diffTop, that makes putting the thing below the text a lot easier than I thought it was going to be. I will note that on my Android phone, the Google Chrome emulation of phones, and the normal desktop site, clicking on the link button now just immediately removes the toolbar. It looks like the url-entering dialog shows up for a frame or two but then is dismissed by some other event, presumably caused by the touchevents code you added in mediumeditor.js.

@lowercasename
Copy link
Collaborator Author

Same on Firefox for the desktop site for me. That is hilariously frustrating, because it works flawlessly in iOS browsers. I copied that code from the mobile branch of medium-editor: https://github.com/yabwe/medium-editor/tree/mobile-support which is 3 years old, so I'm kind of surprised it worked at all. Maybe some of the other code in that branch will improve things?

@lowercasename
Copy link
Collaborator Author

The line about keyboard focus out appears to be... unnecessary? I mean now the bar doesn't go away when you press 'Done' on the iOS keyboard but that's a very very small bug, comparatively speaking. I can't yet get the anchor preview box to appear below the text on iOS, it appears to be ignoring the diffTop setting.

@toBeOfUse
Copy link
Contributor

toBeOfUse commented May 20, 2019

That sounds a little weird. Intriguingly, on my Android phone, the toolbar itself now works fine for adding links, but if you immediately enter a space right after the link it goes away and the text goes back to being "plain" again. This behavior does not replicate on the sweet website (on mobile) as it stands (I'm kind of going around the system text select menu to try this), which makes me suspect that it comes from code in this MediumEditor "mobile" branch.

@toBeOfUse
Copy link
Contributor

Do you want to try adding that line of code that that one guy came up with to the regular MediumEditor script? I'm pretty sure that's how he was doing it.

@toBeOfUse
Copy link
Contributor

No one:
Mobile branch:
image

@toBeOfUse toBeOfUse changed the title Ugly attempt to fix formatting toolbar on iOS Very good and cute attempt to fix formatting toolbar on iOS. Positivity May 20, 2019
@lowercasename
Copy link
Collaborator Author

Okay so I've pushed an update which, on mobile, displays a static toolbar below the editor window, and on browser displays the pop-up we all know and love. Would very much appreciate if you can check how it works on Android! We may have to roll back any weird changes I did to the medium-editor.js before it does work, though.

@lowercasename lowercasename merged commit 277636f into master Jun 10, 2019
@lowercasename lowercasename deleted the mobile-formatting-toolbar branch August 7, 2020 10:24
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.

2 participants