simplesamlphp / SAML-tracer

Browser extension for examining SAML messages
https://addons.mozilla.org/nl/firefox/addon/saml-tracer/
BSD 2-Clause "Simplified" License
142 stars 39 forks source link

Used highlight.js to add syntax highlighting to the content area. #27

Closed khlr closed 6 years ago

khlr commented 6 years ago

Hello everyone!

I thought some syntax highlighting would by quite useful. Hence I used Highlight.js to colorize the content details:

image

Do you all agree to add this functionality? Should there be a button to switch the highlighting on or off (just asking, in case...)?

tvdijen commented 6 years ago

This is great stuff! Personally I don't think this needs a switch as long as we keep it readable for people that are color blind... Might reconsider the color-scheme.

thijskh commented 6 years ago

Yes, very useful. Perhaps best to use the color scheme that Firefox uses for view-source?

khlr commented 6 years ago

I've been very focused on the Firefox color scheme. It nearly completely matches it. @tvdijen, which color scheme would be more appropriate?

tvdijen commented 6 years ago

You could fiddle around with the above image and this website: http://www.color-blindness.com/coblis-color-blindness-simulator/

Epecially if you select Red-blind or Green-blind, everything becomes pretty much one color and thus hard to read. Would it be an idea to keep te current color-scheme by default and be able to switch to an alternative scheme?

P.S.: I'm not color-blind myself, so I wouldn't now what exact colors we need... I was just thinking out loud when I first commented on this PR.

khlr commented 6 years ago

That site is pretty useful for getting an impression of what a colorblind person may see. Thank you, I didn't know it before, but I think I'll use that for other projects, too!

You're right, the red-blind case is one which yields nearly no contrast:

image

But fortunately, the contrast is still quite ok for the other blindnesses, I think (even green-blind).

There's one very positive thing: Even the red-blind view is way better than the status quo (having dark gray text only). What I mean to say is that there are no disadvantages to people who are color blind, but that some just won't benefit so much from the advantages of the syntax highlighting.

Hence I'd try to avoid an on/off-button if it's not necessary, because I think we should strive to keep the UI as lightweight as possible. Do you agree?

tvdijen commented 6 years ago

Aye!

khlr commented 6 years ago

If all agree, I think this PR should be ready for a merge 😃 @thijskh, would you be so kind and do a review again?

jaimeperez commented 6 years ago

Thanks a lot for this @khlr!

Since getting back the import/export functionality was more important than a cosmetic enhancement like this, I merged that PR first. Unfortunately, that means now that we have conflicts in this PR so it can't be automatically merged. Do you mind taking a look at it and fixing the conflicts so that I can merge it?

jaimeperez commented 6 years ago

Wonderful! Thanks again for your hard work @khlr!