New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Error message panel added #82

Closed
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@CodeMaxx
Copy link
Contributor

CodeMaxx commented Jan 30, 2016

Added the basic functionality. Need some pointers on the design of the panel showing the errors.

Review on Reviewable

@hejbot

This comment has been minimized.

Copy link

hejbot commented Jan 30, 2016

Thanks for the pull request, and welcome!

The following reviewer was randomly selected for this PR: @jgraham
And the following additional people are watching it:

@martiansideofthemoon

This comment has been minimized.

Copy link
Contributor

martiansideofthemoon commented Jan 30, 2016

Works for me. Left a few comments. @jgraham will have a better idea about the CSS and what the UI should be 😄


Review status: 0 of 3 files reviewed at latest revision, 3 unresolved discussions.


index.html, line 9 [r1] (raw file):
nit: space on either side of =


index.html, line 139 [r1] (raw file):
Maybe we could add an <a> tag here to change the way the mouse looks? It will be more intuitive I suppose. Also, add a space before ;


index.html, line 148 [r1] (raw file):
nit: This is not a great class name. Maybe something like errorPanel or bottomPanel sound better.


Comments from the review on Reviewable.io

@martiansideofthemoon

This comment has been minimized.

Copy link
Contributor

martiansideofthemoon commented Jan 30, 2016

Reviewed 3 of 3 files at r1.
Review status: all files reviewed at latest revision, 3 unresolved discussions.


Comments from the review on Reviewable.io

@martiansideofthemoon

This comment has been minimized.

Copy link
Contributor

martiansideofthemoon commented Jan 30, 2016

attempts to fix #13

@CodeMaxx

This comment has been minimized.

Copy link
Contributor

CodeMaxx commented Jan 30, 2016

@martiansideofthemoon In the html file there were no spaces around '=' so I kept it like that

@martiansideofthemoon

This comment has been minimized.

Copy link
Contributor

martiansideofthemoon commented Jan 30, 2016

@CodeMaxx , the = is between JS code

@CodeMaxx

This comment has been minimized.

Copy link
Contributor

CodeMaxx commented Jan 30, 2016

A tag will make the result( ERROR , SKIP etc) get underlined. Instead I'm thinking of adding some CSS to change the pointer. I hope thats not a problem.

@martiansideofthemoon

This comment has been minimized.

Copy link
Contributor

martiansideofthemoon commented Jan 30, 2016

@CodeMaxx , yeah didn't think of that. Should do!

@martiansideofthemoon

This comment has been minimized.

Copy link
Contributor

martiansideofthemoon commented Jan 30, 2016

r+ from my side. One nit to fix. Won't merge till @jgraham sees this.


Reviewed 2 of 2 files at r2.
Review status: all files reviewed at latest revision, 4 unresolved discussions.


index.html, line 139 [r2] (raw file):
nit: space after ;


Comments from the review on Reviewable.io

@jgraham

This comment has been minimized.

Copy link
Collaborator

jgraham commented Jan 31, 2016

Reviewed 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 5 unresolved discussions.


index.html, line 139 [r1] (raw file):
You can change the cursor using CSS rather than abusing HTML.

Why do you need to stop the event propagation?

You can't use id=status because id should be unique and this is repeated many times. But we already have class=result so you don't need to add any more ids or classes.


index.html, line 148 [r1] (raw file):
I guess I'd prefer detailsPanel since we might want to add more in the future (stack, test duration, etc.)


wptview.css, line 21 [r3] (raw file):
So I think the style of this is not quite right :) I think it would be nice actually to show more details in the panel (repeating some of the details from the table view), something like:

======================================================= [X]
Test: /path/to/test.html Subtest: 
Expected: PASS Got: FAIL
Message: assert_equals expected 1, got 2 check number
                   of events

And the panel should have a white background, be opaque, have a top border, and cover the full width of the screen (see https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=a05aefe2c48a&selectedJob=20791532 or something for a clearer idea of how it should look).

Ideally we could also find some way to indicate which cell is selected.


Comments from the review on Reviewable.io

@CodeMaxx

This comment has been minimized.

Copy link
Contributor

CodeMaxx commented Feb 1, 2016

All done except highlighting the cell which is seen.
@jgraham I'm using stopPropagation to prevent displayError.visible from becoming false when I click on an Error, since its parent div makes the parameter false, the panel remains invisible on clicking on the error.
The sequence of events occurring in absence of stopPropagation is showError() then displayError.visible = false

@CodeMaxx

This comment has been minimized.

Copy link
Contributor

CodeMaxx commented Feb 1, 2016

@jgraham How do you want the clicked error to be highlighted? (change in background color, a thick border around the box etc.)

@jgraham

This comment has been minimized.

Copy link
Collaborator

jgraham commented Feb 2, 2016

How about using the outline property?


Reviewed 2 of 3 files at r4.
Review status: 2 of 3 files reviewed at latest revision, 8 unresolved discussions.


angular_scripts.js, line 131 [r4] (raw file):
Why not just have scope.displayError.result and in showError do $scope.displayError.result = result?


index.html, line 149 [r4] (raw file):
This feels like it should be a <dl>, although getting the layout might be tricky in that case. It's probably OK to leave like this for now and make the layout better in a followup.


index.html, line 151 [r4] (raw file):
Maybe the error should be in monospace font?


wptview.css, line 21 [r3] (raw file):
You need to allow for the possibility of overflow in this panel (e.g. by adding scrollbars). Note that you probably don't want these to overlap the scrollbars for the main page, so you probably want to rethink the implementation a bit (e.g. put the main view and the popup in independently scrolling divs)


wptview.css, line 28 [r4] (raw file):
You just want a border on the top, I think and it can be written border-top: solid black;


Comments from the review on Reviewable.io

@jgraham

This comment has been minimized.

Copy link
Collaborator

jgraham commented Feb 9, 2016

Reviewed 1 of 3 files at r4.
Review status: all files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.


Comments from the review on Reviewable.io

@martiansideofthemoon

This comment has been minimized.

Copy link
Contributor

martiansideofthemoon commented Feb 25, 2016

@CodeMaxx , could you rebase here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment