-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
feat: add panels hiding hooks to context for plugin use #2952
base: main
Are you sure you want to change the base?
feat: add panels hiding hooks to context for plugin use #2952
Conversation
🦋 Changeset detectedLatest commit: 3e2f67b The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Hey there @matijagaspar 👋, thanks for the comprehensive PR! Is this related to #2929? If so, can you help us understand the need for more than one solution and why the workaround isn't working for y'all? Also, I'm on Discord if you'd like to chat. |
Regarding #2929 we found out is not really suitable to our needs. The customization there is so the One reason for the use of context is to keep the plugin contained and not interfere with any other plugin that might be added to the array too. The other reason is that this specific plugin has a serverside counterpart, that is a plugin to mercurius, and the default setup does not allow us to specify what props mercurius can pass to the You can take a look at the example on the repo: https://github.com/nearform/mercurius-federation-info-graphiql-plugin/tree/master/example However #2929 might still be of use to others and that is why we are keeping it open. Also this PR (#2952) does not close the #1620 and does not provide any additional customization to the users of the So in essence this PR adds an additional API to the plugin API, while the #2929 adds customization options to the |
302989b
to
3e2f67b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @matijagaspar 👋 I found this PR which has been open for quite a while (sorry for not coming back to this earlier 🙏 ). First question: Is the topic even still relevant for you?
Looking at the proposed code changes, I'm not sure I feel comfortable exposing this level of "implementation details". I agree that there is a use-case for programmatically controlling plugin sizes. However, exposing the specific plugin resize elements creates additional API surface that we need to maintain and that we can't simply break without publishing a new major version. I don't have a better idea at this point though 😕
Add the resizable panel state and setters to the context, so plugins can hide/show resizable panels and react on the changes in their visibility.
The context exposes the following object:
A plugin can than use the said context to set hidden element or react on the change of it.
For example (inside a plugin):
This would allow the plugin to show as "full screen" hiding the editor by default.
The names and function of the variables mirror the ones in the GraphiQLInterface, which is a bit confusing, but this is how it works:
For each of the 3 resize objects, the hidden element can either be
null
frist
second
. Null means none are hidden and both are shown, forfirst
andsecond
tells which part of the panel is hidden (see screenshots)pluginResize
editorResize
editorToolsResize