-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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(apps): edit app source form #9795
Conversation
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.
left one comment and will let @Twixes review the frontend code - this certainly looks good to me, in the aesthetic sense
Co-authored-by: Yakko Majuri <38760734+yakkomajuri@users.noreply.github.com>
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.
This definitely makes sense overall. My only comments are about the tabs pattern in PluginSourceTabs
, though I applied the changes as 1ec693f in a stepping-on-toes way.
Just for context though, why not use Ant Tabs
as we don't have an in-house Tabs
style formed yet?
Before
After
const { currentFile, fileNames, pluginSourceValidationErrors } = useValues(pluginSourceLogic) | ||
|
||
return ( | ||
<LemonRow style={{ padding: 0 }}> |
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.
This doesn't need to be a LemonRow
, especially since the child buttons are LemonRow
-based already. We just need flexbox.
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.
This was even in storybook, so 🙈. Thanks!
{fileNames.map((fileName) => ( | ||
<LemonButton | ||
key={fileName} | ||
type={currentFile === fileName ? 'secondary' : 'tertiary'} |
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.
These types don't perform super well here:
tertiary
is intended for really "extra" actions, which we don't want to clutter up the screen too much for users (like "Show details" on insight cards) – but tabs are a key element of navigation, so we definitely want those buttons to look clickable- the
secondary
button is outlined, which gives it a ton of individuality, making the pattern feel less like tabs and more like disparate actions
I suggest using the default type for cohesiveness and instead marking the active file with theactive
prop.
return ( | ||
<LemonRow style={{ padding: 0 }}> | ||
{fileNames.map((fileName) => ( | ||
<LemonButton |
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.
A gap of 0.5rem
between those tab buttons would be perfect
Thanks for the review and the stepping and the ✅. Will merge. Side-question: Other than having read through |
TBH if you read |
* bind logic also in VerticalForm * use a kea form and tabs for plugin source editor * Update frontend/src/scenes/plugins/edit/pluginSourceLogic.tsx Co-authored-by: Yakko Majuri <38760734+yakkomajuri@users.noreply.github.com> * Apply `PluginSourceTabs` suggestion from code review Co-authored-by: Yakko Majuri <38760734+yakkomajuri@users.noreply.github.com> Co-authored-by: Michael Matloka <dev@twixes.com>
Problem
The plugin source drawer, with its multiple code editors, won't scale when we add support for editing apps. It also uses the old antd form, and was in need of a refactor.
Changes
How did you test this code?
Played around in the interface. The API didn't change and the new frontend sends the right data.