usbarmory / interlock

INTERLOCK - file encryption and HSM front-end
Other
295 stars 46 forks source link

Signal UI improvements. #31

Closed vloup closed 7 years ago

vloup commented 7 years ago

I tested the signal app and here is a couple of improvements I would suggest, each in its own commit:

Comments or suggestions are welcome (I'm not really familiar with jquery, css and js).

danbia commented 7 years ago

@vloup Thank you very much for your commit, I'll review it today or tomorrow and come back on this.

danbia commented 7 years ago

@vloup the JS code looks fine apart from the comparison operators != and == that I would replace with !== and === respectively.

Regarding the proposed changes to the CSS, they improves the situation in the case of long lines containing withe spaces, but the horizontal overflow would still be exhibited in the case of very long lines without spaces (for example long links).

Something like this would solve the issue in all the cases as far as I can see:

.history_contents { height: 338px; overflow-y: auto; white-space: pre-wrap; word-wrap: break-word; word-break: break-all; }

In this case the height can be kept to 338px since the horizontal scroll bar should never appear.

Let me know if you can prepare a new clean pull request with these modifications, I will be happy to merge it.

Thanks again for contributing

danbia commented 7 years ago

@vloup , just a small correction on my previous comment the previous height of .history_content was 337px not 338px , so the final CSS would be:

.history_contents { height: 337px; overflow-y: auto; white-space: pre-wrap; word-wrap: break-word; word-break: break-all; }

vloup commented 7 years ago

Thanks for the review! I updated the pull request from your suggestions. One thing remains: if I do not set height: 333px;, I still get this extra scroll bar. I quickly fired up chromium and observed that this bar is not there. Weird...

Here's what I'm talking about, with a lot of gaussian blurs to hide stuffs: https://imgur.com/a/xakxx

danbia commented 7 years ago

@vloup , thanks for the updates to the pull request.

I see the issue related to the height attribute, it is probably related to the way your Firefox is rendering the fonts or similar. If this is the case please modify the height to 333px (as in your original commit), this setting will be more conservative in any case.

I think that after this last change we will be ready to merge the pull request.

Thanks again.