-
Notifications
You must be signed in to change notification settings - Fork 0
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
Allow registration of custom buttons #60
Conversation
Signed-off-by: Joe Fusco <[email protected]>
Signed-off-by: Joe Fusco <[email protected]>
Signed-off-by: Joe Fusco <[email protected]> # Conflicts: # src/components/Editor.jsx
Signed-off-by: Joe Fusco <[email protected]>
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.
see my comment about the query
query_arg.
Then, I think my P.O.C might have been a bit too naiive.
I think we might want to abstract a few things to make this more "error proof" so to speak.
For example, instead of requiring each Button to import and know about the <ToolbarButton>
, <Icon>
and <VisuallyHidden>
components, we could include those elements within our object.entries.map()
and render the component based on "props" in the toolbarButtons
.
for example, when I do add_menu_page()
in WordPress, I pass along strings of data, like: page_title
, menu_title
, menu_slug
, etc and the UI is built for me.
I think ultimately we probably want that type of DX.
The buttons will all be given a consistent look, a tooltip, etc. . .all you need to provide is the label, the icon, the tooltip description and an onClick callback.
So the end user might do something like:
registerGraphiqlToolbarButton({
name: "share",
label: "Share current document",
icon: () => { return <SomeIcon /> },
description: "The tooltip description",
onClick: () => { // this executes when the button is clicked }
});
I, of course, never mentioned that this is the type of API I would like to see, so definitely no reason you should have expected to provide it, but I think ultimately that's where this should go.
Similar to register_graphql_field
, register_post_type
, register_taxonomy
, etc
I'm working on a similar style "registry" API for the "plugins" to add the "Help" plugin, so hopefully that can shed light on the implementation details I have in mind a bit more.
re: Then the list, after any addition/removal has been done, is rendered to the toolbar. |
💯 I am with you on abstracting things out more, but in a follow up PR. Related: #60 (comment) What you have laid out makes good sense to me. |
@josephfusco I opened a follow-up issue here: #62 |
Signed-off-by: Joe Fusco <[email protected]>
@josephfusco looks like a test needs updated to reflect the change to the functionality (moving from multiple supported url query params to 1 hashed param) |
Signed-off-by: Joe Fusco <[email protected]>
Signed-off-by: Joe Fusco <[email protected]>
Signed-off-by: Joe Fusco <[email protected]>
Signed-off-by: Joe Fusco <[email protected]> # Conflicts: # package-lock.json # package.json # src/components/Editor.jsx
Signed-off-by: Joe Fusco <[email protected]>
Signed-off-by: Joe Fusco <[email protected]>
Signed-off-by: Joe Fusco <[email protected]>
Signed-off-by: Joe Fusco <[email protected]>
Signed-off-by: Joe Fusco <[email protected]>
Signed-off-by: Joe Fusco <[email protected]>
Signed-off-by: Joe Fusco <[email protected]>
Signed-off-by: Joe Fusco <[email protected]>
Signed-off-by: Joe Fusco <[email protected]>
Solves #31
This also registers a new share button as an example, using @wordpress/icons package 🎉
notify user (currently console.log)notification system is out of scope of this PRProps to @jasonbahl for the POC 🙌🏻