Skip to content
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

Floating headers in HTML output #2657

Closed
wants to merge 1 commit into from

Conversation

mattparlane
Copy link
Contributor

Hi there.

I recently ran Rubocop over a decent sized existing project and it worked great, but I found it a minor pain trying to figure out what file a particular offence related to. A lot of offences are similar, for example favoring . over :: for static method calls, and I found myself searching the HTML output for files which committed that offence and if a file had (say) 30 offences (not uncommon), I would have to scroll up a long way to figure out which file I needed to change.

This PR is my first attempt and I'm aware it's not complete/polished, but I have found it very helpful and I was hoping to start a discussion about the merits of this kind of feature.

Thanks!

@bbatsov
Copy link
Collaborator

bbatsov commented Jan 18, 2016

I think this would be useful, so I'd accept a polished version of this PR.

@mattparlane
Copy link
Contributor Author

Great, thanks for that. Any hints on what kind of polish? Obviously making the test suite pass would be step 1, but what else? Have you tried it in a browser? What did you think of the UI?

@mattparlane
Copy link
Contributor Author

Also, what level of browser support do you deem acceptable? For example, the classList API does not work in IE9 and below. I will try to make it only progressive enhancement, ie it should retain the current behaviour in all browsers.

@bbatsov
Copy link
Collaborator

bbatsov commented Jan 19, 2016

Great, thanks for that. Any hints on what kind of polish? Obviously making the test suite pass would be step 1, but what else? Have you tried it in a browser? What did you think of the UI?

A changelog will be needed as well. Haven't had time to play with it, so you can just post a screenshot/animated gif.

I'm fine with not supporting IE9 as I doubt many devs are using it.

@mattparlane mattparlane force-pushed the master branch 2 times, most recently from 8275020 to 1459899 Compare January 20, 2016 03:09
@mattparlane
Copy link
Contributor Author

Alright, I've tidied up the formatting, added some comments, tested in IE (it works), and updated the test suite. Pull?

@bbatsov
Copy link
Collaborator

bbatsov commented Jan 20, 2016

You'll have to rebase on top of the current master.

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

Successfully merging this pull request may close these issues.

2 participants