feat: manage provisioner tags in template editor #11600
Conversation
|
What do you think about moving to the left sidebar? |
|
I spoke with @BrunoQuaresma in slack and he suggested this button group and popover approach. If we have support for it in main I'll gladly move the component, but otherwise we can always refactor later. I have a feeling I'm going to ask for a frontend person to take a pass at this component, at least the stylings, when it's ready to review. |
|
I think it looks pretty good as it is and I would move it into the sidebar later. |
|
@BrunoQuaresma I plan to write storybooks as well but I wanted to get an initial review to make sure everything is structurally sound while I get the PR mergable. Thanks |
site/src/pages/TemplateVersionEditorPage/ProvisionerTagsPopover.tsx
Outdated
Show resolved
Hide resolved
site/src/pages/TemplateVersionEditorPage/ProvisionerTagsPopover.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
I left a few comments and I think this is good to go. My only concern is the lack of testing, at least for the main scenario:
- Click on the button
- Add two tags
- Submit the form
- Check if the request was made correctly and the UI is correct
So, if you are comfortable trying it, I would appreciate it.
|
Yup I plan to storybook and then take a stab at testing functionality. Wish me luck lol. |
|
@BrunoQuaresma I've added storybook and jest tests, I'm pretty happy with what they cover but feel free to add any more suggestions as you see fit. Thanks again for the reviews. |
| @@ -106,6 +106,7 @@ | |||
| "@storybook/addon-links": "7.5.2", | |||
| "@storybook/addon-mdx-gfm": "7.5.2", | |||
| "@storybook/addon-themes": "7.6.4", | |||
| "@storybook/preview-api": "7.6.9", | |||
There was a problem hiding this comment.
This is for the useArgs hook I'm using to update state of a rendered component in storybook.
| import { useArgs } from "@storybook/preview-api"; | ||
|
|
||
| const meta: Meta<typeof ProvisionerTagsPopover> = { | ||
| title: "pages/ProvisionerTagsPopover", |
There was a problem hiding this comment.
It is not a page. I would leave it as a component/ProvisionerTagsPopover
| args: { | ||
| tags: MockTemplateVersion.job.tags, | ||
| }, | ||
| render: function Render(args) { |
There was a problem hiding this comment.
Since you are passing the component you don't need this render function at all. At least if you are using it just to test with Chromatic.
There was a problem hiding this comment.
I wanted this to function in Storybook live, and my understanding was that this was the way to manipulate args getting passed into the component with useArgs().
Closes #11360