Bug 1169340 - Perf Compare should validate revisions before loading

[MikeLing]

[jmaher]

doing a drive by here- this looks good! I am happy to see this fixed:) How does this work for the end user? Is there a screenshot of how this looks? When we have the error, this is when they hit the compare button, not when the input happens. I am not sure if we need it to be dynamic.

How will this work if the url is entered with invalid values? Maybe that could be addressed in another PR or bug- maybe this solves it as well or could easily do so.

[MikeLing]

Hi jmaher,

it's pity that I can't paste screenshot for now, because it's midnight in here and I'm afk already. But I can paste them tomorrow: )

How will this work if the url is entered with >invalid values?

Do you mean put revision number in URL directly? Hm, I'm not sure about it actually. Maybe I could try it and reply you after then.

Thank you

[MikeLing]

Hi jmaher, @jmaher

I will paste some screen blow, please fell free to tell me your opinion, thank you! :)

When I type error revision number but haven't type compare button.

Let's press it ;)

It can also deal with single error

I also try put revision number directly, and it will jump into this page :)

[jmaher]

These look great! I think a followup bug should be to make invalid URL's
end up on comparechooser with the correct error message :) Out of scope
for this.

On Sun, Jun 7, 2015 at 2:06 AM, Tiramisu 1993 notifications@github.com
wrote:

Hi jmaher, @jmaher https://github.com/jmaher

I will paste some screen blow, please fell free to tell me your opinion,
thank you! :)

When I type error revision number but haven't type compare button.
[image: before i type compare]
https://cloud.githubusercontent.com/assets/2166227/8022547/ce9bfc72-0d0a-11e5-8730-e9aa24084f7c.png

Let's press it ;)
[image: with both error]
https://cloud.githubusercontent.com/assets/2166227/8022808/e114ea72-0d1d-11e5-9bcc-1ba8c0e858db.png

It can also deal with single error
[image: one error]
https://cloud.githubusercontent.com/assets/2166227/8022810/09ba825c-0d1e-11e5-83d7-c1c74c8ecc9d.png

I also try put revision number directly, and it will jump into this page :)
[image: put it in url d]
https://cloud.githubusercontent.com/assets/2166227/8022817/46f6509c-0d1e-11e5-83d0-5bb7096ba22b.png

—
Reply to this email directly or view it on GitHub
#588 (comment).

[tojon]

Curious @MikeLing did this get run through a js linter? To my (untrained :)) eye, there appears to be some indentation issues above.

[tojon]

Let's press it ;)

Maybe outside the scope of this bug, but it seems a bit unnecessary to say "No results for revision 7atf afaftda6ffa6df on mozilla-central" when both values are directly above the error?

For simplicity I would think

• "Invalid revision"
• "No results"
• "Revision not found"

or similar, as an error might be sufficient here.

[tojon]

@MikeLing @jmaher I had a chance to also just test this branch now (post-merge). Fwiw a couple of other things I think could be improved :) :

The default landing appearance surrounds the inputs with red warning borders, when I haven't done anything yet that is 'bad'. Imo it should only surround with red after I submit a 12 character SHA which is not available after the submit?

(default landing appearance)

Bit of an edge case below, but the form doesn't appear to do alphanumeric checking for the SHA format. For example I can enter these 12 character values, but afaik they are bogus SHA's with the trailing special character *. The 'Compare Revisions' button should not turn dark blue unless the 12 characters are either letters or numbers.

And lastly, if I enter some bad SHA's and hit submit, and then enter some new SHA's, the warnings persist until I click Compare Revisions again. Note I have entered new, valid SHA's below that differ from the stale warnings.

Takeaway: I wonder if we even need the lengthy text dialogue in red. Wouldn't just using red input field surrounds, a tiny red "(!)" above the top right corner of each input field and a tooltip message on each input field "Revision not found" be sufficient?

[MikeLing]

Hi @tojonmz ,
I do have a version commit before that can show blue when you put right and show red when your revision is wrong.

But, the method I use is put a "ng-class" like "ng-class="{'has-error':originalRevisionError}"", but Will tells me he is not sure if it's appropriate method and I should follow the bootstrap document. So I just change it back.

[tojon]

Fwiw, this is my take on what it should look like after a submit where no revisions are found, per my ealier comment and screen grabs:

Takeaway: I wonder if we even need the lengthy text dialogue in red. Wouldn't just using red input field surrounds, a tiny red "(!)" above the top right corner of each input field and a tooltip message on each input field "Revision not found" be sufficient?