Closed sknolin closed 8 years ago
Thanks @sknolin! Apologies for the delay in replying, been travelling and such ;)
This looks great, I've only got one comment, then we're good to merge.
Thanks, I attempted to make this a little more correct.
Cool, thanks @sknolin - just one last nitpick and then I can merge it :)
Also, could you squash the commits? I can do it if need be when i merge it, but it's better if you do, so it doesn't look like I changed your work :)
+1 just what I was looking for (files path)
Sorry for the delay - for the record @sknolin GitHub doesn't notify on new commits, so a comment reminds me to come back and review the new code ;)
I've squashed and merged this as 2582250 - thanks for your patience and contribution!
No problem, I meant to squash and resubmit a merge request, that's why I did a new commit. But didn't get to the squash part yet.
Thanks! Scott
On 11/30/2015 3:39 PM, Greg Sutcliffe wrote:
Sorry for the delay - for the record @sknolin https://github.com/sknolin GitHub doesn't notify on new commits, so a comment reminds me to come back and review the new code ;)
I've squashed and merged this as 2582250 https://github.com/GregSutcliffe/foreman_column_view/commit/2582250655ff75c26163bdf2a064a888c0c6b14c
- thanks for your patience and contribution!
— Reply to this email directly or view it on GitHub https://github.com/GregSutcliffe/foreman_column_view/pull/19#issuecomment-160770245.
I think it's nice if the readme points at the configuration file name/location.
Also had to find out about 'params' option in closed issue #9 from last year. Why not just put it in the readme?