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

Add Statistics Page for top 10 editors #184

Merged
merged 7 commits into from Mar 17, 2018

Conversation

@akhilesh26
Copy link
Contributor

commented Feb 23, 2018

Problem

Collective information of activities is not present on the website.

Solution

A statistics page is added.

Areas of Impact

scripts/build-client-js.sh
src/client/components/pages/index.js
src/client/components/pages/statistics.js
src/client/controllers/statistics.js
src/server/routes.js
src/server/routes/statistics.js
statistics

@coveralls

This comment has been minimized.

Copy link

commented Feb 23, 2018

Coverage Status

Coverage increased (+0.2%) to 57.076% when pulling ce34fbe on akhilesh26:statistics into 876e59a on bookbrainz:master.

@coveralls

This comment has been minimized.

Copy link

commented Feb 23, 2018

Coverage Status

Coverage increased (+0.2%) to 57.092% when pulling be0ae2b on akhilesh26:statistics into 876e59a on bookbrainz:master.

@akhilesh26 akhilesh26 force-pushed the akhilesh26:statistics branch from ce34fbe to 052926b Feb 23, 2018

@LordSputnik
Copy link
Member

left a comment

Great work, I've left a few comments here and there, but nothing major needs changing. Looking at the content you're displaying, it might be useful to add the user registration date, so we can take this into account when looking at the number of revisions.

@@ -118,6 +118,15 @@ class IndexPage extends React.Component {
Develop
</Button>
</Col>
<Col sm={3}>

This comment has been minimized.

Copy link
@LordSputnik

LordSputnik Feb 25, 2018

Member

I'd rather add a link to the statistics page in the Navbar than in this section on the main page, as it's not something people are typically looking for the first time they land on the site.

* @returns {ReactElement} a HTML document which displays the Statistics
* page
*/
class StatisticsPage extends React.Component {

This comment has been minimized.

Copy link
@LordSputnik

LordSputnik Feb 25, 2018

Member

Since this component has no state, it would probably be better as a stateless functional component - see the first example on https://reactjs.org/docs/components-and-props.html

this.renderTopEditorsTable = this.renderTopEditorsTable.bind(this);
}

renderTopEditorsTable(topEditors) {

This comment has been minimized.

Copy link
@LordSputnik

LordSputnik Feb 25, 2018

Member

It would be good to split this into a separate component at the top of the file - maybe called "TopEditorsTable". Then you can just do:

<TopEditorsTable editors={topEditors}/>
<div>
<PageHeader>Statistics of BookBrainz</PageHeader>
{topEditors.length &&
<div>

This comment has been minimized.

Copy link
@LordSputnik

LordSputnik Feb 25, 2018

Member

No need for the wrapping

elements here - these already exist within the sub-component.

*/
const getTopEditors = new Editor()
.query((q) =>
q.orderBy(_.snakeCase('totalRevisions'), 'desc')

This comment has been minimized.

Copy link
@LordSputnik

LordSputnik Feb 25, 2018

Member

I'd just change this to:

q.orderBy('total_revisions', 'desc')

To avoid calling _.snakeCase on a literal.

const markup = ReactDOMServer.renderToString(
<Layout {...propHelpers.extractLayoutProps(props)}>
<StatisticsPage
topEditors={props.topEditors}

This comment has been minimized.

Copy link
@LordSputnik

LordSputnik Feb 25, 2018

Member

Could just pass topEditors directly here, rather than accessing it from props.

@akhilesh26

This comment has been minimized.

Copy link
Contributor Author

commented Feb 25, 2018

@LordSputnik Thank you for the review. I am working on your suggestions :)

@akhilesh26

This comment has been minimized.

Copy link
Contributor Author

commented Feb 25, 2018

View after changes :)
changededitorstable

@akhilesh26

This comment has been minimized.

Copy link
Contributor Author

commented Feb 28, 2018

@LordSputnik If you have time kindly review changes so that I will go to add next stats table.

@LordSputnik
Copy link
Member

left a comment

I've requested another couple of small changes. Thanks for addressing all the things I raised so far.

One thing - could you update the commit messages to follow http://karma-runner.github.io/1.0/dev/git-commit-msg.html . Please make sure that the "type" and "scope", and the first letter of the rest of the message are all lowercase. You can follow this guide to help update the messages (force pushing is fine to do here, since it's a pull request).

https://help.github.com/articles/changing-a-commit-message/

* page
* Renders the document and displays the topEditors table.
* @returns {ReactElement} a HTML document which displays the topEditors table
* in thre statistics page

This comment has been minimized.

Copy link
@LordSputnik

LordSputnik Mar 8, 2018

Member

Typo here - should be "the"

</thead>
<tbody>
{
editors.map((entity, i) => (

This comment has been minimized.

Copy link
@LordSputnik

LordSputnik Mar 8, 2018

Member

I'd change this to editors.map((editor, i) => (

<tr key={entity.id}>
<td>{i + 1}</td>
<td>
<a

This comment has been minimized.

Copy link
@LordSputnik

LordSputnik Mar 8, 2018

Member

Can put this element all on one line

}

StatisticsPage.displayName = 'StatisticsPage';
StatisticsPage.propTypes = {
topEditors: PropTypes.array.isRequired
};
TopEditorsTable.propTypes = {

This comment has been minimized.

Copy link
@LordSputnik

LordSputnik Mar 8, 2018

Member

Better to move this up just under the TopEditorsTable itself.

<div>
<h2 className="text-center">Top 10 Editors</h2>
</div>
<Table

This comment has been minimized.

Copy link
@LordSputnik

LordSputnik Mar 8, 2018

Member

If the line would be under 80 characters, you can put all the tags on one line :)

akhilesh26 added 4 commits Feb 23, 2018
feat: add a new page `statistics` with top editors table
Add new a new page `statistics` with the top editors.
Change the code on both server and client to implement this.
style(client): move statistics button to the navbar
Move statistics button to navbar from the center of the homepage.
refactor(client): convert class `statisticsPage` to stateless function.
Create component for topEditorsTable.
Add column `registration date`  on topEditorsTable.
refactor(server): remove _.snakeCase() from literal `totalRevisions`
Remove the use of _.snakeCase() from the literal `totalRevisions`.
Use `total_revisions` directly.

@akhilesh26 akhilesh26 force-pushed the akhilesh26:statistics branch from 2eaedf5 to 4d7279e Mar 9, 2018

@akhilesh26

This comment has been minimized.

Copy link
Contributor Author

commented Mar 9, 2018

@LordSputnik thanks for the review. I have done all updates as your suggestion. :)

@LordSputnik LordSputnik merged commit 7368e4a into bookbrainz:master Mar 17, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.2%) to 57.092%
Details
@LordSputnik

This comment has been minimized.

Copy link
Member

commented Mar 17, 2018

Looks good to me! Merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.