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

Fixes: #286: Added TwitterFollowersInsight App #291

Merged
merged 1 commit into from Aug 27, 2017

Conversation

Projects
None yet
4 participants
@kavithaenair
Member

kavithaenair commented Aug 6, 2017

Short description

Fixes #286
Demo link: https://kavithaenair.github.io/TwitterFollowersInsight/

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.

@kavithaenair kavithaenair changed the title from [WIP] Fixes: #286: Added TwitterFollowersInsight App to Fixes: #286: Added TwitterFollowersInsight App Aug 8, 2017

@kavithaenair

This comment has been minimized.

Member

kavithaenair commented Aug 8, 2017

@vibhcool

Please see these suggestions for susequent PRs or in this PR:-

  1. Empty page doesn't looks nice, could you add a intro page be added?
    or show any default output
  2. IMHO username shall change on clicking the button, but not on typing.
  3. Also for users with large no. of followers can have hidden. you can show limited followers and a more link to unhide rest followers

views about the suggestions?

@kavithaenair

This comment has been minimized.

Member

kavithaenair commented Aug 9, 2017

@vibhcool thanks for the suggestions, will work on it. How about creating separate PR for these features?

@vibhcool

This comment has been minimized.

Member

vibhcool commented Aug 9, 2017

yes, sounds good :)

rebase needed

@mariobehling

This comment has been minimized.

Member

mariobehling commented Aug 9, 2017

It is unclear what additional value this app is providing compared to sidebar of loklak.org e.g. here http://loklak.org/search?query=from%3Afossasia

We already have this and you are simply replicating it. Does not make sense.

Please compare existing closed source services and this with your work. Here sth. comparable that would be expected as an outcome of a student in the third term: https://moz.com/followerwonk/analyze/fossasia

Which services have you looked at?

A page with more user information such as followerwonk would make a good app. Right now I see a very rudimentary replicate that would have been great before GSoC.

@mariobehling

This comment has been minimized.

Member

mariobehling commented Aug 9, 2017

As @vibhcool points out correctly: Furthermore, you cannot expect your co-developers to follow up on each single point about design and functionality. An app that already sticks to common standards should be provided by you after so much time.

@kavithaenair

This comment has been minimized.

Member

kavithaenair commented Aug 9, 2017

@mariobehling Sorry for not making the app up to the mark. I will enhance this app with more features which will provide more information. Thank you.
For now am changing the PR to WIP stage. Will come up with atleast few good features in this PR.

@kavithaenair kavithaenair changed the title from Fixes: #286: Added TwitterFollowersInsight App to [WIP] Fixes: #286: Added TwitterFollowersInsight App Aug 9, 2017

@kavithaenair kavithaenair changed the title from [WIP] Fixes: #286: Added TwitterFollowersInsight App to Fixes: #286: Added TwitterFollowersInsight App Aug 23, 2017

@kavithaenair

This comment has been minimized.

Member

kavithaenair commented Aug 23, 2017

@mariobehling @vibhcool @djmgit Please re-review.
In the next PR, I will be adding few more new features. Thank you.

@djmgit

This comment has been minimized.

Member

djmgit commented Aug 24, 2017

PR looks good, please look into the following problems.

scrol

Some more padding towards the left of the label will look good I guess. It is almost touching the edge.
Also there is nothing in the page right now, yet there is horizontal scroll.

emptyprob

If I enter a name and hit search it shows result. If I do not enter anything it shows popup. Next time when I enter something and hot enter it again shows popup.

@kavithaenair

This comment has been minimized.

Member

kavithaenair commented Aug 24, 2017

@djmgit yes, will look into horizontal scroll. But am not able to reproduce the second problem you mentioned.

@kavithaenair

This comment has been minimized.

Member

kavithaenair commented Aug 24, 2017

@djmgit Removed the horizontal scroll bar.
image
image

@djmgit

This comment has been minimized.

Member

djmgit commented Aug 25, 2017

err_there

@kavithaenair the error is still there

steps to reproduce

  • Select the input field and press enter without entering anything, popup appears
  • After the above step, enter something and press enter
  • Search happens but the popup appears again

Problem does not occur when button is pressed
When empty input is provided and button is pressed, no popup is shown

@kavithaenair

This comment has been minimized.

Member

kavithaenair commented Aug 27, 2017

@djmgit Resolved it. Please review.

@vibhcool

nice job 👍

@djmgit

djmgit approved these changes Aug 27, 2017

@kavithaenair popup is working now, nice work 👍

@mariobehling mariobehling merged commit e90ed3a into loklak:master Aug 27, 2017

1 of 2 checks passed

codacy/pr Not so good... This pull request quality could be better.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@kavithaenair kavithaenair deleted the kavithaenair:tweet branch Aug 27, 2017

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