regardscitoyens / the-law-factory

Track the french law-making process
https://www.lafabriquedelaloi.fr
GNU Affero General Public License v3.0
75 stars 15 forks source link

Articles: add diff preview #187

Closed njoyard closed 6 years ago

njoyard commented 8 years ago

This PR is a proof-of-concept for article diff preview. It is only a graphical thing for now.

I don't want to get further into it without having reviews on the idea :)

Tell me how you feel about it, here's how it looks: preview

RouxRC commented 8 years ago

I really like it! It deserves to be in prod the soonest, but there are a few bugs left to handle:

Regarding the other points:

clicking on it could scroll down to the part but indeed with the exisiting scrollbar it's probably overkill

if we wanna go towards clickability then this is the way to go, except we need to handle cases when there's long text but no diff

that would be the logical continuation of adopting the second option above, otherwise having the diffviewer's height limited to the actual text's would be preferrable (but hardly clean as well)

cf list above

yes indeed now that colorblind exists, it could certainly apply to the green/red of the articles view which is clearly not best for colorblinds

PS: then the next step I believe is to have a column diff instead of the inline one (or exchangeable?) when the sidebar is unfolded ;) I kinda remember the lib we use for the diff has methods that do exactly this but the doc/examples online disappeared unfortunately

fmassot commented 8 years ago

Really cool feature, +1

Regarding the click interaction on the "diff preview bar", even if the scrollbar is right next to it, it's common in diff viewer to click on it for two reasons according to me : you really want to click on it and click on the "diff bar" move the text exactly where you want to.

I think it's ok if the diff view is still here even for short article and prefer to keep a consistent interface for all articles. Juste like diff preview in diff viewer.

njoyard commented 8 years ago

Okay so I think I finished the implementation.

Now the diff preview, when visible, completely replaces the scrollbar and should have the same features (drag & drop, follow mouse scroll). Clicking on the bar outside the handle brings the cursor to the clicked position, which is different to what most UI scrollbars do ie. step the cursor towards the clicked direction. I think this is more appropriate here. Note that if you don't release the mouse button, you can drag the handle.

Unchanged, added or removed articles have no diff preview, hence they use the browser scrollbar when necessary. Diff preview is also hidden when the whole diff is visible without a need for scroll.

Tested with expanded mode, webkit and gecko. Should work with a touch screen, not tested much though (only touch simulation on PC). Colors are confirmed correct for red/blue colorblind people.

Maybe we'll want to adjust the handle look. I tried using just borders but it was not as legible IMO.

Here is an updated preview:

preview 2

RouxRC commented 8 years ago

Again, really like it, but ;)

njoyard commented 8 years ago

The position is inexact because the text that is compared is not how it is displayed. To be exact, the text would need to be displayed in monospaced font, without paragraphs, and without word-wrap (ie. break in the middle of a word at a fixed character count on each line).

As a consequence, the differences are:

This is most visible on long articles where the cumulative error gets very big the more you scroll. The header also adds an offset but most of the time it should be unnoticeable.

There may be solutions to this :) I think you'd see the same issues on a diff-viewer program.

Or maybe we should not count characters, but instead count paragraphs (alineas)... This may deserve to be tested.

RouxRC commented 6 years ago

i think it's about time to integrate this great feature :D