-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Populate defaults on creating source plugins #4688
Conversation
@@ -69,7 +69,7 @@ export function PluginDrawer(): JSX.Element { | |||
} else { | |||
form.resetFields() | |||
} | |||
}, [editingPlugin?.id]) | |||
}, [editingPlugin?.id, editingPlugin?.config_schema]) |
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.
@mariusandra - based on my 30 minute crash course on Kea, this feels a bit wrong. Shouldn't there be a listener
for the editingPlugin
action which sets the form fields?
I'm not quite sure how the form
would get passed there, but I spent waay too long trying to figure out where in the logic to find the offending piece.
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.
I think this is OK, that is passing a form into Kea feels even wronger to me, and there's no other way to affect it in a listener.
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.
Hmm, I'm guessing because form is a frontend component, it shouldn't make it into the logic ever? <- I can get behind this.
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.
Actually, generateApiKeysIfNeeded
action takes the form
as a parameter, too. 🤔
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.
Yeah, antd
forms try to push us towards a very different way of handling data than what Kea is used to. This useEffect
is a way to somewhat tame the two. I don't know what's the best way forward for these changes. It's all rather flimsy, so make sure to test test test.
I do think that frontend forms should probably also follow a data-first mindset (rather than being directed by the template engine aka React), and will eventually look like a modern version of https://kea-v1.netlify.app/guide/forms -- I'm still unsure of the exact API and if it should be a kea plugin or just a function wrapper like in that link.
caption="Plugins enable you to extend PostHog's core data processing functionality." | ||
caption={ | ||
<> | ||
Plugins enable you to extend PostHog's core data processing functionality. You can also{' '} |
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.
My suggestion for the blurb after the first sentence would be "Use verified plugins from the official repository, or build your own."
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.
On cloud atleast, there's no option to "use verified plugins from the official repository" (nor to build for that matter lol). I think the goal behind this is to encourage building your own plugins?
Since everything in the official repository is already available as an option to install.
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.
@Twixes - are you ok with this?
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.
Well, on Cloud you can use repository plugins, just not freely because we control versions (and a few are not public yet). Right that building is definitely not possible on Cloud yet though, maybe this should have a ternary operator based on preflight.cloud
?
For me it's just that "You can also" sounds a bit awkward without an alternative version.
But anyway this is not a blocker at all! Always easy to adjust copy in another PR.
Changes
Defaults weren't being respected for source plugins on creation.
Before: on installing new plugin ->
After: on installing new plugin ->
Plus, a caption change:
Checklist