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

Schema polling feature #922

Closed
wants to merge 47 commits into from
Closed

Conversation

rohit-ravikoti
Copy link
Contributor

Fixes #.

Changes proposed in this pull request:

  • Implements a new feature where a schema can be reloaded on an interval.

screen shot 2019-01-07 at 1 47 30 pm

  • The feature is currently enabled by default and can be configured in the settings tab. A new variable was added to the configuration called schema.enablePolling to toggle the feature.

screen shot 2019-01-07 at 1 50 16 pm

  • When the feature is enabled, the reload icon turns green and has a subtle glow effect.

  • The polling interval is every 2000ms.

  • The schema only updates in the playground's state if there has been a change. The main reason for this is to keep a smooth UI for the docs explorers. Currently, if the schema updates, the search bar in the DOCS tab gets cleared and the content in the SCHEMA tab scrolls to the top.

rajinwonderland and others added 30 commits December 3, 2018 10:42
Fix for additional line-breaks after each item
Schema will now default to true for commentsDisabled
and commentDescription properties
* Schema only updates in state if it has changed
Fix for additional line-breaks after each item
@CLAassistant
Copy link

CLAassistant commented Jan 7, 2019

CLA assistant check
All committers have signed the CLA.

@kuldar
Copy link
Contributor

kuldar commented Jan 14, 2019

Hey Rohit, thanks for the PR!

I’ve been bit out of loop re: schema polling, so I apologise if I’m missing some important points here. But what’s the reason behind highlighting schema polling state as prominently?

To me, it feels more like a “set it and forget it” setting. You probably want it to be on by default, but also have the ability to turn it off for whatever reason.

The current placement and icon of the indicator feels more like a non-responsive refresh button.

I propose we leave the schema polling just as a setting in configuration, and instead of reloading the schema automatically every 2 seconds, we could show a notice on schema changes along with a refresh link. This would help to avoid situations where someone is browsing the schema and gets refreshed midway through.

screenshot 2019-01-14 at 14 12 54

@rohit-ravikoti
Copy link
Contributor Author

@kuldar, thanks for the input! Are you saying that rather than automatically refreshing the schema, we indicate that there is an update and let them click to update it?

@rohit-ravikoti
Copy link
Contributor Author

One thing I would like to note is that we would need to "refresh" the schema to check if the schema has updated. The default behavior for this is that the refresh icon will spin while it is fetching. Perhaps some more details on what behavior you are thinking would help.

@kuldar
Copy link
Contributor

kuldar commented Jan 15, 2019

Sorry, I should've been clearer - my suggestion was to leave the polling on by default (are there reasons why not to?) and then when changes are detected, show the notice for manual reload of the schema.

@rohit-ravikoti
Copy link
Contributor Author

rohit-ravikoti commented Jan 15, 2019

Thanks! I'll look into adding the manual reload for schema and SDL tabs. I have a couple of questions:

  • You were mentioning something about highlighting the schema polling state. Should we just keep a non-interactive refresh icon without any highlighting when polling is enabled?
  • Currently, polling is enabled by default and can be disabled by setting the schema.enablePolling variable to false. Is this behavior okay with you?

Here is how I am imagining it to be working so far based on your feedback:

  • The query editor will automatically update to show the new suggestions.
  • The SCHEMA and SDL tabs will show a "Refresh to see changes" notice to update manually.
  • Refresh icon will not be as prominent as it is now, but will instead be a non-interactive icon.

For the time being, I will get started working toward the changes I mentioned above, but please feel free to make any corrections 😄

@kuldar
Copy link
Contributor

kuldar commented Jan 16, 2019

Yes, I think we're pretty much on the same page.

I originally also suggested to maybe remove the refresh icon/button altogether, but I noticed now that we've always had the refresh button, just on the other side. So I think your proposal in previous comment makes sense - we'd just show the gray refresh icon next to the url and spin it whenever fetching happens. I would probably not make it green though and keep it in the same gray tone as it is currently.

@rohit-ravikoti
Copy link
Contributor Author

Hi @kuldar,
Here are the changes I made according to your feedback:

  • The refresh icon now spins whenever fetching happens. However, it feels a bit distracting and it might make sense to use a non-animated icon.
  • When the schema updates and the SCHEMA tab is open, a "Refresh to see changes" button becomes visible. When the button is clicked, the schema will be updated and the panel will scroll to the top.
  • When the schema updates while the SCHEMA tab is collapsed, the new schema will be automatically used when the tab is opened.

@kuldar
Copy link
Contributor

kuldar commented Jan 17, 2019

Yes, I think that's the perfect behaviour. I'm also not against removing the animation. Thank you! 👍

@huv1k
Copy link
Collaborator

huv1k commented Jan 17, 2019

Everything looks solid from my point of view.

@rohit-ravikoti
Copy link
Contributor Author

Awesome! I'll go ahead and stop the animation and keep the icon static. Let me know if you would like to use a different icon to indicate polling.

@rohit-ravikoti
Copy link
Contributor Author

Decided to go with #934 instead of this one.

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.

4 participants