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.

Sign up for GitHub

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

Jump to bottom

[WorldMoodTracker] Add streaming feature #315

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[WorldMoodTracker] Add streaming feature #315

wants to merge 1 commit into from

Conversation

singhpratyush
Copy link
Member

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
Member Author

@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
Member

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
Member Author

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
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
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
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
Member

djmgit commented Aug 21, 2017

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

@singhpratyush
Copy link
Member Author

@djmgit: Could you please share the display size?

vibhcool
vibhcool reviewed Aug 21, 2017
View reviewed changes
Copy link
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
Member Author

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

@djmgit
Copy link
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
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
Member Author

@djmgit: Made the changes.

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

@vibhcool
Copy link
Member

link isn't working 😅

djmgit
djmgit approved these changes Aug 25, 2017
View reviewed changes
Copy link
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

@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
Member

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
Member Author

@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
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

@kavithaenair kavithaenair kavithaenair left review comments

@vibhcool vibhcool vibhcool left review comments

@Ishaan28malik Ishaan28malik Ishaan28malik approved these changes

@djmgit djmgit djmgit approved these changes

@Achint08 Achint08 Achint08 approved these changes

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