Arvados

• The “browser is missing” alert has right margin but no left margin. I think it does not look pretty due to this unbalanced nature. Can you either remove the right border or add same amount of border on left (preferable)?

Eliminated both borders, so it looks more like a stripe than a box. (This also gets rid of the rounded corners, which looked weird next to the top nav.)

• Do we also want to add “WebSocket” to browser_unspported.js? I am aware that we only subscribe to event log messages if this is available and is not a show stopper.

Good call. Done.

• If I were to ignore the warning message and continue with login, I continue to see the warning on the top of the page. I think this is ok, but just pointing it out.
  • However, I think in this case one minor suggestion. If we could make the message a bit smaller so that it would fit in one line (in most cases), it could be nicer.

I considered some "hide it after the first page" or "hide it after you click the close button" stuff, but the point of this is to make sure it's impossible to forget (or not know) that you're using Workbench in a way that is known to be broken. I think this goal is incompatible with an "ok, I get it, get that out of my way so I can concentrate" feature.

IOW, improving the experience for people who use Workbench in old browsers is out of scope here. The experience is bad: until/unless we fix that, we should make sure they know why it's bad and how they can fix it.

• BTW, in the alert message do you want to change "you're running an old version..." to "you are running..."? I think it looks too casual this way.

Done.

• Do you want to rename “browser_unsupported.rb” as “browser_unsupported_test.rb” ? All our other tests append _test and this file name threw me off a few times because I was looking for that suffix among all other opened files.

Done. I thought the _test was redundant since they're all in the test/ directory (similarly I try to name test functions test 'it works' do... instead of test 'test that it works' do...). But consistency is good, and making it easier to keep track of files seems worthwhile.

• Do we want to add tests for Blob and File as well? (If you do this, you might want to rename the existing tests and associated properties to read FileReader instead of File)

Not really. :) Killing the FileReader function (part of the HTML5 "File API") is just a way to make the whole "is it supported?" condition fail, so we can verify that failing the condition really causes the alert to appear.

I feel like maintaining two identical lists of conditions would test our ability to keep the lists synchronized, but wouldn't give us any more confidence that our list of conditions is correct. I think the only worthwhile way to test that the list of conditions is working is to use specific combinations of supported/unsupported features that are known to exist in specific versions of real browsers. (And I don't think this branch should have to wait for that.)

Now at 5923d0f

Applied in changeset arvados|commit:6bf9ae122958b25b4a22447f67fb11cf24765d97.