Mobile improvements

[seballot]

Refs #3697 - toolbar icons for mobile

• for colibris skin: Add a plus button to display missing icons
• for no-skin: restore original behavior: all icons are displayed in multiple rows

Other mobiles improvements:

• Timeslider responsive
• Popups (settings, import/export, ...) responsive
• embedded hyperlink responsive popup

[muxator]

Thanks Sebastian, I'll try to install it tomorrow.

If I understand correctly, this reverts the changes we have done on the legacy skins, and implements the "+" icon in Colibris, isnt'it?

What is the reason for which there is a specific commit for ep_embedded_hyperlink? My interest is in understanding if even other plugins may need fixes.

[seballot]

Hi muxator ! In colibris skin, I added some css to style commonly used plugins as ep_embedded_hyperlink, comments_page, brightcolorpicker etc... Here I just noticed a problem with this particular plugin on mobile. Some other plugins may need fixed (for making them with colibris style, or for responsiveness), but I did not install all the plugins to check. If someday you find a plugin with a UI problem, please assign it to me Have a nice day, Sebastian ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐

…

On Tuesday 17 March 2020 23:06, muxator ***@***.***> wrote: Thanks Sebastian, I'll try to install it tomorrow. If I understand correctly, this reverts the changes we have done on the legacy skins, and implements the "+" icon in Colibris, isnt'it? What is the reason for which there is a specific commit for ep_embedded_hyperlink? My interest is in understanding if even other plugins may need fixes. — You are receiving this because you were assigned. Reply to this email directly, [view it on GitHub](#3726 (comment)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/AEEZCXWHFA55EMBYZQAP7P3RH7X5LANCNFSM4LNZBLTQ).

[muxator]

Sorry, tonight I was hold back by other bugs on the project. I'll work on this tomorrow evening.

[muxator]

Works as expected, and I think is a good improvement to usability.
The commits, are well split, with almost no spurious changes and easy to understand.

I was giving a final overall look to all the changes before pulling everything in, when I stumbled upon this snippet in src/static/skins/colibris/pad.js:

etherpad-lite/src/static/skins/colibris/pad.js

Lines 12 to 19 in dac9811

	var timer;
	// on resize end
	window.onresize = function(){
	clearTimeout(timer);
	timer = setTimeout(checkAllIconAreDisplayedInToolbar, 100);
	};
	setTimeout(checkAllIconAreDisplayedInToolbar, 300);
	setTimeout(checkAllIconAreDisplayedInToolbar, 600);

I am really hesitant to merge something that hooks on a resize event in the default skin. I have seen this cause so many performance/correctness problems in the past.

Nowadays I know that CSS wizards are able to do all sorts of magics with just modern CSS. Is it possible that we can achieve the same effect with pure CSS?

[seballot]

Hi @muxator, thanks for the review !

I tried first using CSS flexbox, but etherpad layout is very "complicated", with a lot of cascading containers, using absolute positioning, using iframes etc... it makes the use of flexbox not straightforward. As I was running out of time, I did not spent too much time on refactor all of this. Also, whenever you hit the core layout, you then need to adjust both no-skin and colibri skin.
And the main container is already positioned by javascript in pad_editbar.js

In a more general perspective, I think the Css code could be refactored, I would be happy to do it, but for that it will be much more convenient to abandon the old skin, and use only colibris skin. There are a lot of bad Css in the default skin, overwritten by colibris css, it make the all thing a bit difficult to understand. Also if we could use SASS it would make the code easier to understand

But not sure we are ready to abandon the old skin, WDYT?

[muxator]

As said in the discussion, hooking on a window resize event is never a good idea. @seballot already explained why he had to do this.

For reference @JohnMcLear had to do the same in 2014 in commit 0685e56 ("working on resize"), for similar reasons. That code is still there in 2020 in the current development version:

etherpad-lite/src/static/js/pad_editbar.js

Lines 146 to 148 in 695c2d2

	$(window).resize(function(){
	self.redrawHeight();
	});

[muxator]

Relying on timing for adjusting the layout is too risky.

We are sure that there will be quirks with different device speed / browser combinations.

This is the real reason for which the PR cannot be accepted in its current form.

[muxator]

This PR hooks another event listener to the window resizing. This is not good, but the real problem is the use of timeouts for performing layout calculations.

If we want to move forward with this modification to the toolbar, we should get rid of the timeouts.
Otherwise, let's move beyond this PR and think about how to modernize the code base.

[JohnMcLear]

In 2014 Responsive CSS was not really a thing :D

[muxator]

[...] I think the Css code could be refactored, I would be happy to do it, but for that it will be much more convenient to abandon the old skin [...]. There are a lot of bad Css in the default skin, overwritten by colibris css, it make the all thing a bit difficult to understand.

I understand. Etherpad is a legacy project that comes from a Minimum Viable Product from 12 years ago. It screams for being rewritten (front end and back end). :) But it also has a user base, so we have to think of them.

Also if we could use SASS it would make the code easier to understand

I am not against adding a build step in the code.

But not sure we are ready to abandon the old skin, WDYT?

I think that if we offer a clear migration path it could be done. The default skin is okay-ish, but Colibris is nicer. If I have to choose, Colibris wins.

Another aspect I am thinking of are plugins:

• they are often not maintained, but as soon as the code base is updated, we discover there was some user somewhere that experienced problems;
• on the other side, we cannot think of internalizing the maintenance of all the plugins in a way similar to what we are doing for selected plugins in Colibris now;
• is it viable to document the steps needed to adapt an old plugin to the new CSS architecture? In this way we would have instruction to point the plugin maintainers to (if they pop up).

As you can see, if we decide to embark on this, it will be a lot of work and it will take time.

You already know this because you worked on the integration of Colibris in the first place. This will be harder, and longer. I will not be always able to do timely review (you also saw this), and no code will be merged without thorough review while I am still the maintainer. And every developer knows how much frustration generates waiting for your code to be accepted.

Let's think a bit about it. If yes wins, let's open a issue where we start planning not only the code, but also the deprecation plan, the migration instructions for the plugins, adapting the tests...

[muxator]

In 2014 Responsive CSS was not really a thing :D

Boston Globe redesign is the most famous responsive redesign, and is from 2011.

It seemed like magic at the time. In 2014 it was possible to do a responsive design (we did it in our internal projects at $work), but for sure it was not possible on Etherpad "peculiar" (to be fair) code base.

[JohnMcLear]

Most of the plugins are mine anyway so with a small army I could easily modernize them.

+1 let's get shit done but disclaimer: read below.. My timeframe is very limited..

I only have until the end of Corona lockdown tho and then I'm back to my normal life AFK <3 :D -- I think ETA where I live is 2 or 3 weeks left. I like to dev but I'd rather be active away from a computer, no offense!

[muxator]

I am not sure at all this would be done in 3 weeks, not WRT my personal time availability.

If @seballot agrees to do this on the terms I described, maybe it would be appreciated a contribution on plugins on your side, when this is done.

[seballot]

Hi !
I tried a bit more, and it's okay I can use flexbox to display correctly the toolbar and the editorcontainer. So I can remove the redrawheight() in pad_editbar.js

BUT, for mobiles icons, I cannot find a way to detect that there are two many icons on the toolbar with pure css, I need javascript (if we had a fixed toolbar I could work around something, but here as each etherpad instance have a different toolbar, it's not achievable I think).

Relying on timing for adjusting the layout is too risky.

I agree, we cannot rely on the timeout to initialize the mobile icons, I will fix. I knew it was dirty but it was late and I made it quick. Never a good idea to rush some coding session :)

But still, I'll be obliged to use the resize event to update the toolbar if someone change the size. I don't understand why you are so afraid of this resize event? seems to me that it's commonly used. I read some issue on stack overflow, and I should better use window.addEventListener('resize', function(event){ // do stuff here });
Instead of window.resize = function....

Re my proposal to refactor, actually I think I can keep the "no-skin", I will just try to refactor it a bit.

The questions of the plugins is an interesting question.

is it viable to document the steps needed to adapt an old plugin to the new CSS architecture?

The architecture will not change, so the plugins will still work. The only thing is that they might be designed with the old fashion style. I cannot make a document to say "hey, please stop having bad taste in term of design" :D more seriously, we can give some instructions like "don't change the font family and the font size inside the plugin, use the inherited one. Use white background etc...", but still**, most developers are not designers, and the result is always very instable**
So to have a nice and homogenous design, I think we are obliged to deal the style of the plugins in a centralized way. But it's likely we cannot do that for all plugins, there are so many...

If it's fine by you, I will begin refactoring a bit no-skin (without breaking changes), and then introduce flexbox and fix the timeout thing. I will wait your opinion re resize event

[muxator]

Now that #3744 is merged is this PR still relevant?

[seballot]

Nop, we can close !