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

Issue 22 #26

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Issue 22 #26

wants to merge 4 commits into from

Conversation

vishadGoyal
Copy link

@vishadGoyal vishadGoyal commented Oct 10, 2020

What type of PR is this? (check all applicable)

  • ♻️ Refactor
  • ✨ Feature
  • 🐛 Bug Fix
  • 👷 Optimization
  • 📝 Documentation Update
  • 🔖 Release
  • 🚩 Other

Motivation

I got here from good first issues. I got there from the Hacktober fest website. 😄

Related PRs and Issues

#22

(If this PR is related to any other PR or resolves any issue or related to any issue link all related PR and issues here.)

@vercel
Copy link

vercel bot commented Oct 10, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/abhisheknaiidu/a-pop/43iqpir4g
✅ Preview: https://a-pop-git-issue-22.abhisheknaiidu.vercel.app

Copy link
Owner

@abhisheknaiidu abhisheknaiidu left a comment

Choose a reason for hiding this comment

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

Excellent, but could you subscribe songs instead of quering? because if we are adding the song and quering it, it's not loading instantly, we'll need to refresh the page to see that songs.

@vishadGoyal
Copy link
Author

That will require changes on the server side. We need to define a subscription there which will notify a client when a new song is added

https://www.apollographql.com/docs/react/data/subscriptions/
https://www.apollographql.com/docs/apollo-server/data/subscriptions/

@abhisheknaiidu
Copy link
Owner

That will require changes on the server side. We need to define a subscription there which will notify a client when a new song is added

https://www.apollographql.com/docs/react/data/subscriptions/
https://www.apollographql.com/docs/apollo-server/data/subscriptions/

Yes I agree with you, but I've defined already, you can look into this - https://github.com/abhisheknaiidu/a-pop/blob/master/src/graphql/subscriptions.js

@vishadGoyal
Copy link
Author

This always returns the full list

Take a look at this one: https://www.apollographql.com/docs/apollo-server/data/subscriptions/

@abhisheknaiidu
Copy link
Owner

This always returns the full list

Take a look at this one: https://www.apollographql.com/docs/apollo-server/data/subscriptions/

ah I got it, so what do you think, what should be better approach in terms of both UI and UX?

@vishadGoyal
Copy link
Author

vishadGoyal commented Oct 11, 2020 via email

@abhisheknaiidu
Copy link
Owner

Can you share the current backend implementation of the subscription? I'm not an expert in GraphQl but after reading the documentation it seems that the best way to go would be to use the paginated query system that I've implemented along with a subscription that returns a newly added song. Since songs are ordered by creation date, new ones will always go to the top and are paginated system will continue appending to the bottom.

On Sun, Oct 11, 2020, 6:44 PM Abhishek Naidu @.***> wrote: This always returns the full list Take a look at this one: https://www.apollographql.com/docs/apollo-server/data/subscriptions/ ah I got it, so what do you think, what should be better approach in terms of both UI and UX? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#26 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AFSBRGP6Q54ZFGNIK6CDPFTSKGVUZANCNFSM4SLKKK5A .

Here it is: https://zepta-music.herokuapp.com/console

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.

2 participants