Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

Added search jslint error with icon #4246

Closed
wants to merge 4 commits into from
Closed

Added search jslint error with icon #4246

wants to merge 4 commits into from

Conversation

drewhamlett
Copy link
Contributor

I think this is what you guys wanted as far as layout and hover.

I used the row-highlight class as the hover detection. I didn't see that class used anywhere else but it maybe wise to add a class id to the jslint table.

Thanks

screen shot 2013-06-15 at 7 29 27 pm

@drewhamlett
Copy link
Contributor Author

There's an issue with opening multiple tabs. Looking into it.

@ghost ghost assigned njx Jun 17, 2013
@@ -54,7 +54,7 @@ html, body {
cursor: default;

/* Turn off subpixel antialiasing on Mac since it flickers during animations. */
-webkit-font-smoothing: antialiased;
-webkit-font-smoothing: subpixel-antialiased;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this diff snuck in (I think you've been making this change locally) - could you remove it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do. Yea I committed some local changes by accident.

@njx
Copy link

njx commented Jun 19, 2013

Cool, this is looking good overall--just a few code comments. Also, the "i" icon looks a bit big and slightly vertically off-center to me. @larz0 - would it make sense to decrease the size of the "i" icon slightly (see screenshot above)?

@larz0
Copy link
Member

larz0 commented Jun 19, 2013

@drewhjava here's the update icon based on @njx's feedback http://cl.ly/0I2b1T3Q1F3F. Thanks~

@njx
Copy link

njx commented Jun 22, 2013

@drewhjava - the new icon looks pretty good. Looks like there are still a few comments from the previous review that need addressing. I'm out for the next couple of weeks, so I'm reassigning this to @gruehle - please add a note when you've dealt with the other comments. Thanks.

@ghost ghost assigned gruehle Jun 22, 2013
@peterflynn
Copy link
Member

Maybe it's not necessary given how great Google is, but I wonder if we should do anything do avoid polluting the search query with parts of the message that are specific to the user's code. E.g. for the JSLint error "'foo' was used before it was defined" you probably don't want to include 'foo' in the search since it's unlikely anyone else will have the same string there. I think we could accomplish that by looking at .raw instead of .reason and then stripping out all the templating symbols. Maybe it's overkill though... Thoughts?

@peterflynn
Copy link
Member

Another thought -- what about linking to http://jslinterrors.com/ ? It looks like its search function gets matches on the unmodified .raw strings (at least if you omit the period at the end of the sentence). Or maybe if that site gets enough love then it will just bubble to the top of all the Google results :-)

@peterflynn
Copy link
Member

@drewhjava Just noticed this when reviewing some old pull requests. Are you still interested in pursuing this?

The linting system now uses a generic UI managed by CodeInspection.js, with JSLint as only one of many potential result providers. So I think we'd need to consider how to change this into a generic way for any provider to specify 'more info' links (or clickable icons in general?).

If you're interested I think we'd be happy to review that change -- but it would be different enough from this patch that I think we'd want to start a new pull request for it. So I'll close this one -- but feel free to open an updated PR any time!

Thanks for your efforts!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants