New Web UI #184

Merged
merged 1 commit into from Nov 23, 2016

Projects

None yet

5 participants

@tejasbubane
Contributor

This is the implementation of the new UI being discussed here: #173
I also took the liberty to upgrade some of the javascript dependencies.

This is a combined effort from @cybrilla and we are loving it ❤️
Run it and let us know what you think.!

Closes: #173

lib/rubycritic/core/analysed_module.rb
@@ -19,6 +23,22 @@ def path
@path ||= pathname.to_s
end
+ def file_location
+ file_path[0...-1].join('/')
@troessner
troessner Nov 18, 2016 Collaborator

Did we actually ever discuss if we support Windows? If yes, this should be File::SEPARATOR.

@Onumis
Onumis Nov 20, 2016 edited Member

I can't see a reason not to. But there are a ton of system commands we rely on that might not work on windows...

@guilhermesimoes
guilhermesimoes Nov 21, 2016 edited Collaborator

It should support Windows (at least I think it used to).

And File::SEPARATOR is unnecessary as an analysed_module has a pathname attribute which is of the type, uh, Pathname 😄

This method should simply be

pathname.dirname
@troessner
Collaborator

@lucasmazza @Onumis are we actually supporting windows? That's something we should clarify and put in our README.
The Ruby code looks good for me to merge, the JS stuff looks ok for me as well with one exception (pls see my comment).

@tejasbubane amazing job, thanks a lot for this!

@Onumis

This is looking really great, thanks for all the effort!

Just a few notes:

  1. assets/images/logo.png the name of this gem is actually just a single word: RubyCritic (no spaces)
  2. Although we don't need to worry about responsive design, I noticed that the design breaks a bit on widths < 1199px, which is still a very common size. I think we should focus on getting the pages to look great on screens >= 992px.

image

+ var textX = chart.plotLeft + (chart.plotWidth * 0.5);
+ var textY = chart.plotTop + (chart.plotHeight * 0.5);
+ var span = '<span id="pieChartInfoText" style="position:absolute; text-align:center;">';
+ span += '<span style="font-size: 32px">'+((score/100)*4).toFixed(2)+'</span><br>';
@Onumis
Onumis Nov 20, 2016 Member

Although smaller numbers look better, I think we should still rely on our 0 to 100 score system (https://github.com/whitesmith/rubycritic/blob/master/docs/core-metrics.md#score)

+ var textY = chart.plotTop + (chart.plotHeight * 0.5);
+ var span = '<span id="pieChartInfoText" style="position:absolute; text-align:center;">';
+ span += '<span style="font-size: 32px">'+((score/100)*4).toFixed(2)+'</span><br>';
+ span += '<span style="font-size: 16px">GPA</span>';
@Onumis
Onumis Nov 20, 2016 Member

And instead of GPA we can display /100 or of 100.

+ <thead>
+ <tr>
+ <% unless Config.suppress_ratings %>
+ <th width="10%" class="table-header">Rating<span class="sort-type"></span></th>
@Onumis
Onumis Nov 20, 2016 Member

I noticed the default order in this table changed from "name" to "rating". And I actually like it, but I think it would be best if the order was reversed, so that the lowest ratings are at the top.

+ <div class="col-md-3">
+ <small><%= @analysed_module.complexity %> complexity</small> <br/ >
+ <small><%= @analysed_module.duplication %> duplications</small>
+ </div>
@Onumis
Onumis Nov 20, 2016 Member

I think this div is duplicated.

lib/rubycritic/core/analysed_module.rb
@@ -19,6 +23,22 @@ def path
@path ||= pathname.to_s
end
+ def file_location
+ file_path[0...-1].join('/')
@guilhermesimoes
guilhermesimoes Nov 21, 2016 edited Collaborator

It should support Windows (at least I think it used to).

And File::SEPARATOR is unnecessary as an analysed_module has a pathname attribute which is of the type, uh, Pathname 😄

This method should simply be

pathname.dirname
lib/rubycritic/core/analysed_module.rb
+ end
+
+ def file_name
+ file_path[-1]
@guilhermesimoes
guilhermesimoes Nov 21, 2016 Collaborator

pathname.basename or pathname.basename.sub_ext('').to_s if you don't want the file extension, as seen in the Location class.

lib/rubycritic/core/analysed_module.rb
+ file_path[-1]
+ end
+
+ def file_path
@guilhermesimoes
guilhermesimoes Nov 21, 2016 Collaborator

I believe this will now be an unnecessary method 😄

lib/rubycritic/core/analysed_module.rb
+ end
+
+ def line_count
+ `wc -l "#{path}"`.strip.split(' ')[0].to_i
@guilhermesimoes
guilhermesimoes Nov 21, 2016 edited Collaborator

This is introducing extra behaviour that may not work on other platforms, I believe.

It's also not caching the value.

Maybe this could be moved to another PR that attempts to solve this in a more cross-platform way?

+ <td><%= analysed_module.churn %></td>
+ <td><%= analysed_module.complexity %></td>
+ <td><%= analysed_module.duplication %></td>
+ <td><%= analysed_module.smells.length %></td>
@guilhermesimoes
guilhermesimoes Nov 21, 2016 Collaborator

We can now use the new smells_count method.

@tejasbubane
Contributor

Review comments fixed, please check @Onumis

@Onumis
Member
Onumis commented Nov 23, 2016

Alright! Looks good to me!
Please, squash changes into a single commit :)

@Onumis
Onumis approved these changes Nov 23, 2016 View changes
@rohitcy
Contributor
rohitcy commented Nov 23, 2016

@Onumis Squashed all the commits, we have also fixed the logo text 'Ruby Critic' to 'RUBYCRITIC'(No Spacing) it looks like this now:

screen shot 2016-11-23 at 8 33 00 pm

@Onumis Onumis merged commit b977b62 into whitesmith:master Nov 23, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Onumis
Member
Onumis commented Nov 23, 2016

Thanks! Gonna release it now! 😄

@tejasbubane tejasbubane deleted the cybrilla:new-web-ui branch Nov 23, 2016
@tejasbubane
Contributor

@Onumis Great! 🎉 Sending a PR to update the docs with new images.

@tejasbubane tejasbubane added a commit to tejasbubane/rubycritic that referenced this pull request Nov 29, 2016
@tejasbubane tejasbubane [Fixes #189] Use Ruby's File instead of `wc` system command 534f72c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment