Closed pauljt closed 10 years ago
I very much like the experiment tab.
The rest of the UX layout restructuring I'm not sure I'm on board with. Obviously, a lot of this is 'favorite color' type arguments, so I'm going to play with the interface and follow-up, but it feels completely different than the original UX branch and I'm not sure I like it. Mainly, a lot of it is fixed not fluid and some horizontal vs vertical issues.
Also, this PR addresses a lot more than UX so we should probably break it apart. For example, the removal of jQuery, acorn file restructure, etc. (I don't think its absolutely necessary, but I can see that becoming hairy when we try to merge ux2 and ux together into some new version)
I've pushed my most recent changes to my ux
branch and am going to spend some time using both of that and ux2
this morning... I'd imagine we will land someplace in between. I might try to get a ux3
branch going that is a version of both of them but I'll follow up to be sure.
:two_men_holding_hands:
Also, I'm pretty sure tests require jQuery so we will at least need it in that directory. I can't recall if tests uses its own jquery already or not though.
app.js
still seems to use quite a bit of jQuery, do we want to change that over as well?
I prefer the 2 column layout of ux2. For me it's easier to quickly view the information I care about.
There are a few ways I think it could be improved:
1) The process of adding files (this applies to ux and ux2). I should be able to use the file selector multiple times without replacing my previous selection. There are plenty of use cases where this is reasonable. A big one would be I just selected 50 files, but forgot the 51st. Currently I'd have to reselect all of them. Another one is that (at least how I have my file viewer on my mac set up), selecting files in different directories is impossible.
2) What is the point of select/deselect? Why would I add files and then decide not to run a scan with them? It seems like the ability to dynamically add and remove files from a list would obviate the need for this, but maybe you guys have something else in mind.
3) Results should be grouped and collapsed. I ran a scan of the files in client/js and got ~25 results. Even this small amount of results is difficult to parse quickly. I think that the left column should closely mirror the left column of the "set up scan" view with some contextual differences. I'd like to see a list of files with perhaps a badge next to them displaying the number of results found for that file. Clicking a file name would cause a list to expand underneath it showing the line number and the rule violated (description could be available via mouseover of the rule. I see no reason why this description needs to be duplicated N number of times on the page. Bootstrap has great looking tooltips that are easy to use) for each of the violations found. In addition to expanding the violations, clicking the file name would cause its source to be displayed in the file viewer in the right column (highlighting the lines that are violations would be a nice plus).
That's all I can think of right now. Not trying to be too specific and get into too much of my opinion, but as someone who wants to use this tool, I think at least some version of the above suggestions would improve the usability. A fundamental rule of design is not make your users think, and a good way of doing this is to not have too much clutter and useless information (of course the opposite extreme is just as bad). It's OK to display just what the user needs right this second and then let them take an (intuitive) action to drill down and get more information. Right now I feel like both ux and ux2 attempt to display everything all at once.
Apologies for the long-windedness. Just wanted to give some more specific feedback after checking out both interfaces this morning. I know some of the things I've mentioned might not be something that needs to be done right now even if everyone agrees it's a good idea, but it might be worthwhile to keep them in mind as the current iteration is being designed. I'm happy to help out with development if y'all have anything you want me to do.
P.S. Why is jQuery forbidden?
Awesome, thanks for the feedback @rbhitchcock!
So @pauljt and I had a long conversation yesterday about the layout and actually discussed a lot of your points. I'll address each of your bullets below, but just to summarize what we discussed yesterday and our current plan:
We are going to take ux
the way it is now and make the following changes:
ux
ux2
removed preetify because it violated CSP because I was lazy, so I need to fix that(FIXED).innerHTML
rule results for exampleux2
to ux
and make it full screen heightDefinitely would love some feedback on the above, but I'll also address your points:
1) Yes, 100% agree. Right now, in ux
, you can kinda do that and it keeps results but its definitely more of a quirk than a feature :).
2) So select/deselect is for a few reasons
js
directory with a bunch of JS files. From there the user can use the left sidebar to inspect all the files, deciding which are or are not interesting to scan. Then click scan and not have to wade through results from files they don't care about.3) I actually really like all these ideas. Not sure it will hit this UX uplift (unless we find surprise PR :wink:) but I also don't really know how work might be involved with that. To address this issue, we plan to implement a filtering of results based on rule so you don't have to see results you're not interested in, but I think the collapsible files with badges sounds sweet. I also think we aren't displaying things 'intelligently' persay, but more so dumping the results, so I think we can improve there for sure.
4) jQuery - It's not really a strict policy, but in Gaia (FirefoxOS UI) they don't use jQuery, and I guess this is mainly an exercise in purity. :smile: That, and, we can/should do a lot of the jQuery things we were doing in Angular functions.
Really appreciate the feedback and please keep the comments coming; the more feedback we can get on how people will use the tool best the better. If you do happen to be interested in helping with ux in the near future, definitely let us know and we can get some issues setup and assigned so we don't conflict. :checkered_flag:
@rbhitchcock brought up a good point that made me consider our revised scan tab that we proposed:
I wondering if we should:
Your proposed UI changes sound like a big step in the right direction. I think doing that will make a big difference for usabilty. After reading your use cases for checkboxes, I would still argue that the ability to add/remove files from the list would address those needs, but the checkboxes in and of themselves aren't that big of a deal. I just think that adding and removing files is a necessary feature, and then once you've got that the checkboxes are redundant. Maybe not, though.
You second comment raises a couple of questions for me. I'm still not in love with the thought of being presented with so many options to choose which rules I want to run the scan with (or which ones i want to remove from results). Would it be enough to offer some presets, e.g., All, Ignore CSP checks, etc. Then, in the results view, rather than presenting me with a huge list of rules that i select/deselect, provide an autocomplete field where I can enter rules that I wish to be filtered. I feel like the most common use case is that there are just 0 <= n < 5 categories that I would want to exclude, so rather than present me with the daunting task of parsing a table/list with 50+ elements, let me tell you what I don't want. The field being autocomplete can provide enough assistance (especially since there is a rules tab I can visit to get a complete listing).
Additionally, if there are options in the scan setup to exclude some rules, why would you still perform these tests? That part doesn't make sense to me.
Awesome, and I'll be sure to keep yah in the loop as we get closer to a final version for some more feedback.
Yah, rules and how we handle them in the backend/frontend is definitely something we still need to 'solve'. I'm not sure this will be a milestone 1.0 but we definitely need to figure our a better way for the user to filter rules.
Regarding:
Additionally, if there are options in the scan setup to exclude some rules, why would you still perform these tests? That part doesn't make sense to me.
Because it would require a large restructuring of the scan engine. Right now, we just pull in /common/rules.js
and use that as the ruleset for both server side version and client version. If we want to actually prevent the rule from being passed to scan, then we need to restructure the backend which isn't in scope for milestone 1.0, but definitely something that will be considered in the rule overhaul.
@rbhitchcock
Great feedback. To comment specifically: 1) Really good idea, i had been focusing on zip files (firefox OS apps) but i think this makes sense for the manual select case 2) So most Firefox OS apps have libraries you dont want to scan included in the zip. This was a way to deselect them before scanning. Hence why we need a disable, not just a remove (to support the case when I load a zip file, and then deselect some files, but then decide I want to add some files back. If we just remove them, they will have to reload the zip to get the file back and there will be duplicates now etc). 3) Yeh we definitely need something like this for results. I also discussed some kinda of "Rule Groupings" with Rob so you could show hide results based on their type/category etc. Personally I've found that sorting via issue was a more logical way to review (at least with Firefox OS apps) but I would like the ability to sort/group via filename too, so something like you suggest makes sense.
Probably work on these as separate patches once I finish merging.
So my previous was to reply to just the first comment 4. I read the rest, and lots of good stuff in here, but Im going to keep this patch at getting the overall structure of the UI done, and lets move changes to file loading, rules grouping and results filtering to separate patch since it sounds like this will need some iteration by the sounds of it.
Agree with all of that. Iterative approach is the best way to go imo. Nice work guys! Excited for this tool.
This could probably be merged with master now if we want. I re-added the sidebar, and added in an issue filter in the sidebar, so you can filter the list of results. (I mainly did this just as an experiement to see if the sidebar was useful to keep and I think it is).
I still need to review and merge the final commits in the ux branch. Hopefully get to that later tonight.
Awesome, this is starting to look sweet! :fist: :fire:
I agree, we should try to get it into master pretty quick so we have a version to base any tweaks off of.
A few functionality things right now:
Fixed those bugs, and refactored a little to make the code easier to read (to avoid making the same type of errors in the future).
PS this last commit also hide the "all issues" line and implemented the behavior you suggested which I think is better. I tested and it seems to work now.
I'm a bit confused. Does this obosolete #96 or are these independent changes that will both go into the source tree?
@mozfreddyb, this one will be merged into master so feel free to ignore #96.
I have some small changes (badges, and fluid based height) to ux2
. I plan to send in a PR against ux2
this morning, then I think we should be good to go to merge ux2
into master
.
The changes my PR will make:
Please feel free to review this branch! Any changes/comments you might have, definitely post comments/suggestions.
I think the final stage for 1.0 is getting the rules and scan engine fixed so we don't have any failures. I'm not sure the overhead involved in that but hopefully its not too bad (I haven't dug into that code yet :open_mouth: )
I've got the code for AST printing sitting around, but that should probably be put off until we figure out how we want to handle rules. I don't see much advantage to the the reviewer to seeing the AST if they aren't going to be editing/adding rules.
Right now, clicking someplace in the header that is not a link (e.g. NOT scan, rules, experiment) it causes the main area to be hidden. Will make the following change to only cause hide when you click an actual link.
diff --git a/client/index.html b/client/index.html
index 9a83c8d..2cda296 100644
/client/index.html
- <ul class="nav navbar-nav navbar-left">
+ <ul id="header-links" class="nav navbar-nav navbar-left">
/client/js/main.js
- document.querySelector("#header").addEventListener("click", function(evt){
+ document.querySelector("#header-links").addEventListener("click", function(evt){
var active=evt.target.id.split('-')[0]+"-wrapper";
LGTM
This pull requests includes all of the changes from the ux branch, and a lot of cleanup work. A summary of the changes:
TODO: some things I know I haven't fixed/done yet: