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

First response of a subscriptions wrongfully clears isFetching state #2774

Open
simhnna opened this issue Sep 23, 2022 · 7 comments
Open

First response of a subscriptions wrongfully clears isFetching state #2774

simhnna opened this issue Sep 23, 2022 · 7 comments

Comments

@simhnna
Copy link
Contributor

simhnna commented Sep 23, 2022

old

@thomasheyenbrock
Copy link
Collaborator

Hey @simhnna 👋 This behavior is intended. The full view should be available once there is a first response. For subscriptions the play button as you noted is currently the indicator if the request is still running or not.

I'm with you that the current state is not ideal:

  • Previous payloads are overridden once new ones come in
  • The UI is lacking in terms of a11y

@thomasheyenbrock
Copy link
Collaborator

I don't think we have an issue yet that tracks the bigger picture, i.e. how we can improve the UI for subscriptions. Would you be up for broadening the scope of this issue? I think I would be better thinking that through first before jumping on smaller items like the loading spinner and the isFetching state. What's your take on this?

@simhnna
Copy link
Contributor Author

simhnna commented Sep 23, 2022

Yeah we can broaden this issue I guess.

I'm no a11y expert, I'd rather leave that to someone else. But yeah I see 3 main pain points with the current implementation:

  1. The execute button doesn't show the correct state for subscriptions (which is what I was mainly interested in fixing)
  2. The loading indicator shown above the results for subscriptions. I could live with that for the time being
  3. No history

Execute button

For our internal deployment of graphiql I'd rather have that fixed quickly, and it's a small change (but it surfaces issue 2...)
We usually have long running subscriptions and currently there is no indicator that the subscription is still going. And IMO the fix improves the situation

The loading indicator

I guess it's well placed for queries and mutations. Subscriptions could be running for a long time, and I don't think reserving that much space is a good idea. Maybe switch over to a loading bar? Not sure where to place it though.
If subscriptions use a loading bar, maybe the queries/mutations should also switch to that...

History

That's a big point. I currently mainly use altair for subscriptions but I can't say I'm happy with how the subscriptions are displayed there. I actually very much like the current implementation in that only shows the most recent value. I do understand that showing the history is something people will want to have.
If a history is added for subscriptions, I'd prefer it to be a separate view that you can switch to. I'd find a history view that can highlight diffs or only show specific keys of the response quite useful. Just having tens of large objects that you need to open/close that jump around when new entries are added isn't that useful to me...

@acao
Copy link
Member

acao commented Sep 29, 2022

@thomasheyenbrock the original issue for improved subscriptions UI is #436. it was in the github project roadmap I shared!

@simhnna
Copy link
Contributor Author

simhnna commented Nov 10, 2022

Could we maybe discuss a MVP so I can get this bugfix in?

@thomasheyenbrock
Copy link
Collaborator

The behavior of the execute button has been changed in the PR linked above ☝️

That change has just been published with with graphiql@2.2.0 and @graphiql/react@0.15.0

@mehulmathur16
Copy link

@simhnna @thomasheyenbrock Hi, is there a way to access the isFetching state & use our own custom loader ?
I'm using version 3.2.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Todo
Development

Successfully merging a pull request may close this issue.

4 participants