Skip to content

Add sorting to Skunk HTML report - #142

Merged
JuanVqz merged 9 commits into
fastruby:mainfrom
poonambhagaur61-tech:sort-skunk-report
Sep 28, 2026
Merged

JuanVqz merged 9 commits into
fastruby:mainfrom
poonambhagaur61-tech:sort-skunk-report

Conversation

@poonambhagaur61-tech

@poonambhagaur61-tech poonambhagaur61-tech commented Sep 12, 2026 •

Copy link
Copy Markdown
Contributor

This PR adds sortable columns to the Skunk HTML report.

  • Allows sorting by File, Skunk Score, Churn × Cost, Churn, Cost, and Coverage
  • Supports ascending and descending sorting
  • Displays the current sort direction
  • Keeps the existing default Skunk Score ordering

Tests: 88 runs, 126 assertions, 0 failures, 0 errors, 0 skips

Scope: sorting applies to the HTML report (skunk_overview.html) only. The console and JSON reports keep their current output, ordered by Skunk Score.

Closes #116

@JuanVqz
JuanVqz self-requested a review September 14, 2026 17:25

@JuanVqz JuanVqz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hey @poonambhagaur61-tech, Thanks for taking the time to contribute. I reviewed and have some questions for the issue creator. Feel free to share your opinions as well.

As for me, the changes looks good in general, so, let's wait for him, but if no response we can decide what to do, thanks!

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

When loading the report, the arrow icons do not show up

Image

They should be visible like so; however, I'm unsure if they show a neutral hint or the actual initial sort preference. The reports start with:
Image

For the neutral hint, I added this .table-header::after { content: " ⇅"; opacity: .35 }

@fbuys,
Would you be happy with a neutral hint or more like an initial arrow that matches the actual pre-sorted order?
And, since you are around, what do you think about the sorting implementation? As for the code, I'd say it looks good, but as for what columns you want to be sorted, What are the criteria to follow, etc.?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the helpful feedback. I’ve updated the report to indicate the initial sort direction. Since the report is initially sorted by Skunk Score in descending order, the Skunk Score column now displays the descending arrow by default, while the other columns remain neutral until selected. I’ve also kept the existing numeric and alphabetical sorting behavior unchanged.

Please let me know if you have any further suggestions or would prefer a different sorting behavior. Thank you again for your guidance!

@JuanVqz
JuanVqz requested a review from fbuys September 15, 2026 20:56
@JuanVqz JuanVqz mentioned this pull request Sep 22, 2026
2 tasks done

@JuanVqz JuanVqz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for the update, @poonambhagaur61-tech! Showing the arrow only on the column being sorted works well, and the report now opens with Skunk Score ▼, which matches the default order.

Please add an entry to CHANGELOG.md under "main (unreleased)", something like: * [FEATURE: Add sortable columns to the Skunk HTML report](https://github.com/fastruby/skunk/pull/142) and address the code changes I left as diff please.

Comment thread lib/skunk/generators/html/templates/skunk_overview.html.erb Outdated
Comment thread lib/skunk/generators/html/templates/skunk_overview.html.erb Outdated
Comment thread lib/skunk/generators/html/templates/skunk_overview.html.erb Outdated
Comment thread lib/skunk/generators/html/templates/skunk_overview.html.erb Outdated
@poonambhagaur61-tech

Copy link
Copy Markdown
Contributor Author

Thank you for the feedback! I’ve added the requested CHANGELOG.md entry under the unreleased section and linked it to PR #142. I’ve also addressed the requested changes. Please let me know if any further adjustments are needed.

Comment thread CHANGELOG.md Outdated
Comment thread CHANGELOG.md
Co-authored-by: Juan Vásquez <javasgon@gmail.com>
Co-authored-by: Juan Vásquez <javasgon@gmail.com>
@poonambhagaur61-tech

Copy link
Copy Markdown
Contributor Author

No problem! I’ve updated the CHANGELOG.md as requested. Thank you for your guidance and feedback throughout the process. I’d be grateful if you could consider merging the PR when everything looks good.

@JuanVqz

JuanVqz commented Sep 28, 2026

Copy link
Copy Markdown
Member

@poonambhagaur61-tech thank you for taking the time to contribute to our gem.

have you ever heard about hacktoberfest? Is going to be running next month if you want to contribute I’ll probably be adding this gem to it, so, if you contribute it will count on there.

@JuanVqz
JuanVqz merged commit 7841c88 into fastruby:main Sep 28, 2026
10 checks passed
@poonambhagaur61-tech

Copy link
Copy Markdown
Contributor Author

Thank you so much, Juan! Yes, I’ve heard about Hacktoberfest, and I’d definitely love to participate. I really enjoyed contributing to Skunk and learned a lot from the review process. I’d be happy to continue contributing to the gem during Hacktoberfest. Thanks again for the opportunity and your guidance!

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.

Add ability to sort output results

2 participants