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

Addresses #635 implemented statistic infobox similar to loklak #704

Merged
merged 16 commits into from Aug 8, 2017

Conversation

nikhilrayaprolu
Copy link
Member

@nikhilrayaprolu nikhilrayaprolu commented Aug 2, 2017

Fixes issue #635

Implemented Statistic infobox that shows top providers, authors etc

Demo Link: http://susper-pr-704.herokuapp.com/

Screenshots for the change:

screenshot from 2017-08-03 01-13-07
screenshot from 2017-08-03 01-12-55

@nikhilrayaprolu nikhilrayaprolu temporarily deployed to susper-pr-704 August 2, 2017 20:17 Inactive

</ul>
</div>

Copy link
Member

Choose a reason for hiding this comment

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

Remove extra line.

{{element.name}} <span class="badge">{{element.count}}</span>
</a>
</li>

Copy link
Member

Choose a reason for hiding this comment

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

Remove extra line.

StatsboxComponent,
],
providers: [

Copy link
Member

Choose a reason for hiding this comment

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

Since there are no providers, we can remove it.

navigation$: Observable<any>;
selectedelements: Array<any> = [];
changeurl(modifier, element) {

Copy link
Member

Choose a reason for hiding this comment

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

Remove extra line.

@harshit98
Copy link
Member

harshit98 commented Aug 3, 2017

@nikhilrayaprolu Please go through the suggested changes which need to be done. Since we're in the final phase of GSoC, it would be great if we maintain code quality too. :)

Fix Travis issue too. Good Work! 💯

Copy link
Contributor

@Marauderer97 Marauderer97 left a comment

Choose a reason for hiding this comment

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

LGTM great work 💯


}
ngOnInit() {

Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove extra line

this.querylook = Object.assign({}, query);
this.navigation$ = store.select(fromRoot.getNavigation);
});

Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove extra line

@nikhilrayaprolu nikhilrayaprolu temporarily deployed to susper-pr-704 August 3, 2017 07:08 Inactive
@nikhilrayaprolu
Copy link
Member Author

@harshit98 @Marauderer97 made the requested changes.

Copy link
Member

@harshit98 harshit98 left a comment

Choose a reason for hiding this comment

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

Good work! 💯

@mariobehling
Copy link
Member

Demo link not working on Chrome. Page is https.
screenshot from 2017-08-03 09-49-12

I can see already the following:

  • Use same colors as exisintg
  • Use same box style and indents

Still need to test the links.

@nikhilrayaprolu nikhilrayaprolu temporarily deployed to susper-pr-704 August 3, 2017 08:56 Inactive
@nikhilrayaprolu
Copy link
Member Author

@mariobehling please use this new deployment link at http://susper-pr-704.herokuapp.com/

Copy link
Contributor

@Marauderer97 Marauderer97 left a comment

Choose a reason for hiding this comment

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

Please resolve travis

@nikhilrayaprolu nikhilrayaprolu temporarily deployed to susper-pr-704 August 3, 2017 18:22 Inactive
@nikhilrayaprolu
Copy link
Member Author

image

@nikhilrayaprolu
Copy link
Member Author

nikhilrayaprolu commented Aug 3, 2017

@mariobehling done with the requested changes removed Protocol and collections since they don't make a meaning for user. I will make the distribution of Date and protocols in a new PR.

@codecov
Copy link

codecov bot commented Aug 3, 2017

Codecov Report

Merging #704 into master will increase coverage by 0.35%.
The diff coverage is 65.57%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #704      +/-   ##
==========================================
+ Coverage   58.94%   59.29%   +0.35%     
==========================================
  Files          46       47       +1     
  Lines         984     1049      +65     
  Branches      102      106       +4     
==========================================
+ Hits          580      622      +42     
- Misses        342      362      +20     
- Partials       62       65       +3
Impacted Files Coverage Δ
src/app/services/speech-synthesis.service.ts 26.31% <0%> (-1.47%) ⬇️
src/app/statsbox/statsbox.component.ts 66.66% <66.66%> (ø)
src/app/auto-complete/auto-complete.component.ts 41.17% <0%> (+1.17%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 561e6ef...4e4bbaf. Read the comment docs.

@mariobehling
Copy link
Member

@nikhilrayaprolu

  • What is the heading of the box? There is none currently?
  • Please position the sidebar boxes more smoothly in the design. The top box is not in the same line as the top text and the second box has a big space. Reduce the space.
  • Where did you get the idea for the gray boxes? They are very dominant. Please propose a less dominant design, that does not attract the attention of the user right away. The main focus of the page should always be the search results.
  • You show the top ten results. Please provide an option to view more results.
  • The first line below "top authors" does not show a name. What is this number related to? Undefined?

@mariobehling
Copy link
Member

Adding this box now also adds another feature to the page that increases the issues with the design of the site in different resolutions. See the screenshot below with a number of display issues on large screens.
screenshot from 2017-08-04 09-05-38

@nikhilrayaprolu nikhilrayaprolu temporarily deployed to susper-pr-704 August 4, 2017 20:43 Inactive
@nikhilrayaprolu
Copy link
Member Author

nikhilrayaprolu commented Aug 4, 2017

@mariobehling updated the PR:

  • implemented more and less button that expands
    less button
    screenshot from 2017-08-05 02-10-46
    more button to get more options
    screenshot from 2017-08-05 02-10-31

  • left no margin b/w both infobox and statistic box, which is more better in user experience.
    screenshot from 2017-08-05 02-11-27

  • The statistic box is self explanatory with titles as Top Provider and Top Authors, so I think we don't need to have a heading to the box.

  • Grey color is changed to green
    screenshot from 2017-08-05 01-46-12

  • removed the undefined author.
    screenshot from 2017-08-05 02-10-31

Regarding the large screen the problem persists on susper.com and not generated in this PR we could solve that issue in #683

[labels]="lineChartLabels"
[options]="lineChartOptions"
[chartType]="lineChartType"
></canvas>
Copy link
Member

Choose a reason for hiding this comment

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

Indent this line.

}

public chartHovered(e: any): void {
console.log(e);
Copy link
Member

Choose a reason for hiding this comment

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

Remove console.log

public lineChartLegend: boolean = true;
public lineChartType: string = 'line';
public chartClicked(e: any): void {
console.log(e);
Copy link
Member

Choose a reason for hiding this comment

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

Remove console.log

@nikhilrayaprolu nikhilrayaprolu temporarily deployed to susper-pr-704 August 7, 2017 19:31 Inactive
@nikhilrayaprolu nikhilrayaprolu temporarily deployed to susper-pr-704 August 7, 2017 19:43 Inactive
@nikhilrayaprolu nikhilrayaprolu temporarily deployed to susper-pr-704 August 7, 2017 19:45 Inactive
@nikhilrayaprolu nikhilrayaprolu temporarily deployed to susper-pr-704 August 7, 2017 19:47 Inactive
@nikhilrayaprolu
Copy link
Member Author

@mariobehling made the requested changes
screenshot from 2017-08-08 01-15-21
screenshot from 2017-08-08 01-15-12
screenshot from 2017-08-08 01-14-55

Regarding a monthly wise statistics, that could be implemented if we could so statistics of all results, but presently we are just showing the results of present page because to retrieving specific fields of a result could be done only when &wt=json but we can't do that when &wt=yjson but JSONP callback is supporting only yjson format. I think we can achieve that with a small change on the server side. @Orbiter
when wt=yjson
screenshot from 2017-08-08 01-18-32
when wt=json
screenshot from 2017-08-08 01-18-06

@nikhilrayaprolu nikhilrayaprolu temporarily deployed to susper-pr-704 August 7, 2017 19:56 Inactive
@nikhilrayaprolu nikhilrayaprolu temporarily deployed to susper-pr-704 August 7, 2017 20:00 Inactive
@nikhilrayaprolu nikhilrayaprolu temporarily deployed to susper-pr-704 August 7, 2017 20:01 Inactive
@harshit98
Copy link
Member

@nikhilrayaprolu Please follow up this commit: here and remove unnecessary import of modules from NotFoundComponent.

@nikhilrayaprolu
Copy link
Member Author

@harshit98 sure, doing that now.

@nikhilrayaprolu nikhilrayaprolu temporarily deployed to susper-pr-704 August 8, 2017 16:54 Inactive
@nikhilrayaprolu
Copy link
Member Author

@harshit98 updated the pull removing all extra modules

@harshit98
Copy link
Member

Looks good to me now! We can merge this PR. Good work! 💯

@mariobehling
Copy link
Member

Thank you! Please open an issue on the server regarding monthly and potentially weekly results. Cheers.

@mariobehling
Copy link
Member

Please change "Protocol Frequency" to "Protocol Distribution" in a follow up PR as well.

@nikhilrayaprolu
Copy link
Member Author

👍

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.

None yet

4 participants