Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

feat: manage provisioner tags in template editor#11600

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
f0ssel merged 11 commits intomainfromf0ssel/manage-tags
Jan 18, 2024
Merged

Conversation

f0ssel
Copy link
Contributor

@f0sself0ssel commentedJan 12, 2024•
edited
Loading

Closes#11360

Screenshot 2024-01-17 at 1 25 16 PM

Screenshot 2024-01-17 at 1 25 27 PM
Screenshot 2024-01-17 at 1 25 06 PM
Screenshot 2024-01-17 at 1 25 00 PM
Screenshot 2024-01-17 at 1 24 46 PM

matifali reacted with hooray emojimatifali and BrunoQuaresma reacted with heart emoji
@matifali
Copy link
Member

matifali commentedJan 16, 2024•
edited
Loading

What do you think about moving to the left sidebar?
I think@BrunoQuaresma may have some thoughts, too, as he is currently working on refactoring the template editor with collapsible sidebars.

@f0ssel
Copy link
ContributorAuthor

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.

matifali reacted with thumbs up emoji

@BrunoQuaresma
Copy link
Collaborator

I think it looks pretty good as it is and I would move it into the sidebar later.

f0ssel reacted with thumbs up emoji

@f0ssel
Copy link
ContributorAuthor

@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

BrunoQuaresma reacted with thumbs up emoji

Copy link
Collaborator

@BrunoQuaresmaBrunoQuaresma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.

@f0ssel
Copy link
ContributorAuthor

Yup I plan to storybook and then take a stab at testing functionality. Wish me luck lol.

@f0sself0ssel marked this pull request as ready for reviewJanuary 18, 2024 18:02
@f0ssel
Copy link
ContributorAuthor

@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",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

What does it do?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

This is for theuseArgs 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",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

It is not a page. I would leave it as acomponent/ProvisionerTagsPopover

f0ssel reacted with thumbs up emoji
args: {
tags: MockTemplateVersion.job.tags,
},
render: function Render(args) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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 withuseArgs().

Copy link
Collaborator

@BrunoQuaresmaBrunoQuaresma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Awesome work! Just one minor question and consideration but not a blocker.

@matifali
Copy link
Member

matifali commentedJan 18, 2024•
edited
Loading

This is shaping up so nicely. Great work@f0ssel 🎉
@bpmct, what do you think about tagging starter templates with tags they have in metadaa? For example, GCP VM templates will have tagsgcp=true,vm=true,linux=true
image

@f0sself0ssel merged commitbf0a6fc intomainJan 18, 2024
@f0sself0ssel deleted the f0ssel/manage-tags branchJanuary 18, 2024 22:35
@github-actionsgithub-actionsbot locked and limited conversation to collaboratorsJan 18, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.
Reviewers

@BrunoQuaresmaBrunoQuaresmaBrunoQuaresma approved these changes

Assignees

@f0sself0ssel

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow viewing and modifying provisioner tags on template versions via the dashboard
3 participants
@f0ssel@matifali@BrunoQuaresma
[8]ページ先頭
©2009-2025 Movatter.jp