Closed seballot closed 4 years ago
Actually, please do not review this PR right now, I will still work a bit on it tomorrow, and also run and fix the tests !
Note that I also worked on the tests so do pull my branch with fixed tests.
I fixed ep_table_of_contents and ep_set_tilte_on_pad, thanks @JohnMcLear for having merged the PR !
Do you see others plugins that might break the UI? I mean plugins which insert content inside the main layout?
ep_webrtc ?
ep_comments / ep_comments2?
Thanks ! Is there an easy way have ep_webrtc working for real in local? Or do you know an online etherpad instance using this plugin?
@seballot just install ep_webrtc and install some certs from letsencrypt on a domain. You need to run https, if not maybe it will work in exceptions?
I just have this empty container for webrtc (I put it on the right)
anything on console?
WebRTC ERROR: DOMException
will check.
Hrm, try remove other plugins and try against latest develop (lots of change lately on develop).
It works for me without anything in settings.json
, I just see
Sorry, your browser does not support WebRTC.
To participate in this audio/video chat you have to user a browser with WebRTC support like Chrome, Firefox or Opera. Find out more
I could add a protocol check and if it's http mention why it's not working... I just expect everything to be on https nowadays but I guess if you are running locally...
I'd remote any ep_webrtc settings too, might be a bug in that?
Thanks ! but if I just have this message it will not help very much :) I wanted to see the others components of the plugin (like where the video appears etc...) to ensure everything works. You do not know an online instance?
Unfortunately settings https on local host in a bit a pain
Amazing ! thanks :) https://video.etherpad.com/p/test
Can I have a brief recap on the status of this and #3726?
Hi muxator !
For this one : etherpad core is now using flexboxes to easily positioning element on the layout (no more absolute positioning with javascript like $(editbox).css('top', $editbar.height())
It simplify a lot the code (both javascript and css) but this change is not compatible with plugins introducing others components in the layout with absolute positioning. I'm am currently fixing those plugins (table_of_content, set_tilte_on_pad, webrtc). But it introduce a compatibility problem between etherpad core version and plugin version, see discussion here about webrtc : https://github.com/ether/ep_webrtc/pull/28#issuecomment-604698324. Your help would be welcome to find a good solution
What remain to be done in this PR is fixing the colibris skin
re #3726, I will wait this refactor to be done so I could rebase the changes on top of it
@muxator @JohnMcLear I gave more thoughts re the problem of compatibility between core code and plugins
Seems to be that the only way to improve the codebase without compatibility problem is to :
WDYT?
I don't mind dragging people up to 1.9 to implement the new CSS.
The point is we need to make it clear WHY something is broken if it is, currently it fails silently. That's not good enough.. console.warn("upgrade yo shit"); for example...
@muxator WDYT about John proposal? It's a easy solution, fine by me
Hi @seballot, I am ok with your proposal, above all because it outlines a clear deprecation plan for the old features.
I also agree whith @JohnMcLear: when on 1.9 the plugins are going to break, there must be a clear message somewhere that at least points to the documentation on how to fix them.
My only curiosity is how would we do that.
Wow you are working, man :)
wow reviewing this is going to be a lot of work :)
Yes sorry ! And I'm not finished yet ah ah :)
But lots of commit only involved css, so they should no be very critical to review
@seballot is this wrapped up?
nop, I'm working on it right now, I did some improvement on the ep_comments_page, and now I'm styling it with colibris
Just few more things and it will be ready, hopefully before the end of the week
I want to make this part of the next release (1.8.3), there is enough material for warranting a release now. Writing the changelog is going to take half a day alone.
Ok ! My changes are finally ready... UI is really a black hole, very time consuming, and also it never end... So I'm ending it for now :)
One thing I couldn't find the energy to do is fixing the tests. I do not know this test language, and with the refactor I guess a lot of specs will break (mostly due to refactor, not to functionality downside)... Does someone would like to fix them?
So main improvements of this PR are
#skinvariantsbuilder
to the urlHere are some screens
Wow! I will look at tests now.
Initial testing. Gotta say it looks amazeballs and works great. Just some minor polish required :D
Timesliders revision test can be fixed with
var latestContents = timeslider$('#padcontent').text();
// Click somewhere on the timeslider
var e = new jQuery.Event('mousedown');
e.clientX = e.pageX = 150;
e.clientY = e.pageY = ($sliderBar.offset().top -38);
$sliderBar.trigger(e);
e = new jQuery.Event('mousedown');
e.clientX = e.pageX = 150;
e.clientY = e.pageY = ($sliderBar.offset().top -33);
$sliderBar.trigger(e);
e = new jQuery.Event('mousedown');
e.clientX = e.pageX = 150;
e.clientY = e.pageY = ($sliderBar.offset().top -43);
$sliderBar.trigger(e);
the stick chat option is there for me, not for you?
What I did is remove this option on mobile, because it does not make sense
Hrm, I see, well that's new behavior and will prolly upset some people who want chat to be a focus on browsers that use 1/2 a screen.
I'm not -1 on it, but if you think about it, depending on the test runners setup the experience of if the browser thinks it's mobile or not will change..
The test will need to be more explicit, essentially saying what width the browser width is of the tester.. Our tests are automated across browsers and across operating systems, so we can't set the screen resolution fixed... I will check that tho
To replicate the bug..
See what I mean?
Looks like the runner draws the iframe on the screen, perhaps we can set width there.. Either way we can fix the tests for sticky chat after this is merged in..
On pad.js
line 567,
sticky option disappear for screens below 800px. You can still have the chat visible on the bottom (it take 100% of the width), but it will no longer stick on the right, otherwise the editor became too small
The problem is that when running the specs, the editor take only half width of the screen, so yes on normal monitor test screen seems like almost like a mobile screen
for running the specs, maybe we could allow the page to be bigger than 100% width (horizontal scrollbar) and so we can set the iframe to a certain width if needed?
@seballot yeah, defo something we can fix "after". No problem from my side. Focus on the Pad Modal stuff? Afaik I submitted a patch for disconnect too, check PRs, I might have broken it the same as you :D
Ok re pad modal, you are talking about the "gritter popup" right? like the save revision, or when a message in the chat is received?
Allow the user to select all pad contents when the disconnect dialogue is visible
Indeed nice feature broken
Yea the gritter popup, If you click save revision, you'd expect it to dissapear whe nyou click back on pad or so.. Chat msg will need to persist, I think it's the location & duration that's a problem... Thoughts?
From what I saw about gritter in etherpad there are two options : 'sticky' and 'non sticky'
the non sticky disappear after some time (configurable) the sticky stay until you click the close icon
I added the close icon also on the non sticky one, so you can close them if you want. Closing them also with a pad click is an option yes, I'm just wondering that user close them by mistake (if you are in the middle of an action, and the gritter pops during your action, it will directly disappear). It was like this before? I tried on develop branch, but the gritter message is not even poping up... lol
re chat gritter, if we are talking about the gritter to say that someone wrote a chat msg when my chatbox is hidden, I don't think they need to persist (i.e. they should be non sticky, disapear themself after 3second or so), WDYT?
I agree, but I think save revision can be there for like 2 seconds tops, else it could be really annoying taking up the viewport.
Chat msg is dynamic information you need to read every time. Saved revision is static informaiton you will never read after you read it once.
That's the point to consider here :)
Yes good point !
So what shall we do? does adjusting the time each notification stay visible is good enough for you? like chat msg for 4s, and save revision for 2s?
‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐ On Friday 17 April 2020 17:30, John McLear notifications@github.com wrote:
I agree, but I think save revision can be there for like 2 seconds tops, else it could be really annoying taking up the viewport.
Chat msg is dynamic information you need to read every time. Saved revision is static informaiton you will never read after you read it once.
That's the point to consider here :)
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.
Yea could be a division of the setting value. Try different ones and see what feels right
Hi ! I fixed all specs (except the slider one), and also the ability to copy the pad text when we are disconnected
re the the pad_modal test, actually is was not the gritter popup, but the normal popup like settings or so, and the problem was just how we detect that those popup are open or not
But still made some improvement for gritters
Wow great job!! I absolutely can't wait to show people this work
It's hard to express the amount of work @seballot has done on this PR. It's gargantuan.
I'm focusing on the js changes:
farbtastic
and gritter
are two old vendorized libraries that this PR edited. I've added a note on top of those files (Edited by Sebastian Castro [...] on...
) to track the fact that they are no longer the original version. This may come in help when in need of reconstructing the history of those files;src/static/js/chat.js
. I'll address that later.Overall, looking at this PR's diffstat for the js files, it's interesting to observe that the amount of javascript code has indeed decreased, despite the introduction of new functionalities (skin variants, animations).
We should be looking to contribute to gritter and farbtastic with some changes instead of going off piste.
I'm gonna write some front end test code that finds every input and tries to inject some xss
we should be looking to contribute to gritter and farbtastic with some changes instead of going off piste
In general, I totally agree. In this case, these libraries were already vendorized: hopefully who decided to vendorize them had already done a cost/benefit analysis. We are paying that cost now.
The skin variants builder at http://localhost:9001/p/test#skinvariantsbuilder is super cool.
We have to mention it somewhere prominent!
As we discuss earlier with @muxator and @JohnMcLear here is a pull request to improve the css code base
Main idea was to simplify the code, i.e. stop using absolute positioning lead by javascript, stop using table for positioning elements, and use flex box instead to make the design more responsive and auto adjustable
Note re the pull request