Skip to content

[WorldMoodTracker] Add streaming feature#315

Open
singhpratyush wants to merge 1 commit intoloklak:masterfrom
singhpratyush:314
Open

[WorldMoodTracker] Add streaming feature#315
singhpratyush wants to merge 1 commit intoloklak:masterfrom
singhpratyush:314

Conversation

@singhpratyush
Copy link
Copy Markdown
Member

@singhpratyush singhpratyush commented Aug 20, 2017

Short description

Fixes #314.

Changes:

  • Modal to display Tweets on clicking a country.
  • Start and stop streams from triggers at different components.

Link: https://singhpratyush.github.io/world-mood-tracker/index.html

Screenshots for the change:

screenshot from 2017-08-20 21 39 51

I have:

  • There is a corresponding issue for this pull request.
  • Mentioned the Issue number in the pull request commit message Fixes #<number> commit message
  • There is only strictly only one commit per issue.

For the reviewers

I have:

  • Reviewed this pull request by an authorized contributor.
  • The reviewer is assigned to the pull request.

@singhpratyush
Copy link
Copy Markdown
Member Author

singhpratyush commented Aug 20, 2017

@Achint08 @djmgit @hemantjadon @kavithaenair @SKrPl @vibhcool: Please review.

djmgit
djmgit reviewed Aug 20, 2017
View reviewed changes
WorldMoodTracker/js/components/StreamOverlay.js Outdated
if (this.eventSource) {
return;
}
// this.eventSource = new EventSource(host + '/api/stream.json?channel=' + 'all');
Copy link
Copy Markdown
Member

@djmgit djmgit Aug 20, 2017

Choose a reason for hiding this comment

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

You may remove this.

Copy link
Copy Markdown
Member Author

@singhpratyush singhpratyush Aug 20, 2017

Choose a reason for hiding this comment

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

@djmgit: Removed the line.

djmgit
djmgit reviewed Aug 20, 2017
View reviewed changes
Copy link
Copy Markdown
Member

@djmgit djmgit left a comment

Choose a reason for hiding this comment

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

App is displaying tweets, nice work 👍 Please see my comment inline.

kavithaenair
kavithaenair reviewed Aug 21, 2017
View reviewed changes
Copy link
Copy Markdown
Member

@kavithaenair kavithaenair left a comment

Choose a reason for hiding this comment

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

How can i get back to the page which was showing map, from here:
image

@singhpratyush
Copy link
Copy Markdown
Member Author

singhpratyush commented Aug 21, 2017 •
edited
Loading

@kavithaenair: You can click the area between the modal and address bar to hide it.

@djmgit
Copy link
Copy Markdown
Member

djmgit commented Aug 21, 2017

@singhpratyush the modal is not being properly displayed on mobile screen.

@singhpratyush
Copy link
Copy Markdown
Member Author

singhpratyush commented Aug 21, 2017

@djmgit: Could you please share the display size?

vibhcool
vibhcool reviewed Aug 21, 2017
View reviewed changes
Copy link
Copy Markdown
Member

@vibhcool vibhcool left a comment

Choose a reason for hiding this comment

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

Works fine for me. but shouldn't the host link be set to Loklak?

@singhpratyush
Copy link
Copy Markdown
Member Author

singhpratyush commented Aug 21, 2017

@vibhcool: Currently, streaming is not enabled for the main deployments of loklak. That's why I am using this deployment.

@djmgit
Copy link
Copy Markdown
Member

djmgit commented Aug 21, 2017

world2

modal is working fine, I guess it was some other problem, its working fine now, please have a look at this issue. Words are moving out of the boundary. Not sure about the exact break point, I encountered it at around 350px.

@djmgit
Copy link
Copy Markdown
Member

djmgit commented Aug 21, 2017

Also in your subsequent PRs can you put a close button on your modal? I guess it would improve the UX as the area to click on in order to hide the modal is very less :)

@singhpratyush singhpratyush force-pushed the 314 branch 2 times, most recently from 13e4581 to 138c2e3 Compare August 22, 2017 19:52
@singhpratyush
Copy link
Copy Markdown
Member Author

singhpratyush commented Aug 22, 2017

@djmgit: Made the changes.

@Achint08 @hemantjadon @kavithaenair @SKrPl @vibhcool: Please review.

@vibhcool
Copy link
Copy Markdown
Member

vibhcool commented Aug 22, 2017

link isn't working 😅

djmgit
djmgit approved these changes Aug 25, 2017
View reviewed changes
Copy link
Copy Markdown
Member

@djmgit djmgit left a comment

Choose a reason for hiding this comment

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

Nice work 👍

Achint08
Achint08 approved these changes Aug 25, 2017
View reviewed changes
Copy link
Copy Markdown

@Achint08 Achint08 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 @singhpratyush :)

@mariobehling
Copy link
Copy Markdown
Member

mariobehling commented Aug 25, 2017

This is all great, but we don't have reliable search results that the server provides. Therefore the service works unreliably. Please help to make all GSoc projects a success and ensure everyone passes. The pre-requisite for this is: The search results need to work reliably.

@singhpratyush singhpratyush force-pushed the 314 branch 2 times, most recently from e17757f to 49e6426 Compare August 26, 2017 22:43
@singhpratyush
Copy link
Copy Markdown
Member Author

singhpratyush commented Sep 3, 2017

@mariobehling: We've increased the timeout in order to get better search results in the server.

But this pull request is not related to the search results in any way. Please merge this.

Thanks.

Ishaan28malik
Ishaan28malik approved these changes Feb 2, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@Ishaan28malik Ishaan28malik left a comment

Choose a reason for hiding this comment

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

It works fine

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

5 more reviewers

@vibhcool vibhcool vibhcool left review comments

@kavithaenair kavithaenair kavithaenair left review comments

@Achint08 Achint08 Achint08 approved these changes

@djmgit djmgit djmgit approved these changes

@Ishaan28malik Ishaan28malik Ishaan28malik approved these changes

Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@singhpratyush @djmgit @vibhcool @mariobehling @Achint08 @kavithaenair @Ishaan28malik