Skip to content

Updated kci-dev command generator - #2084

Open
aliceinwire wants to merge 5 commits into
kernelci:mainfrom
aliceinwire:kcidev
Open

Updated kci-dev command generator#2084
aliceinwire wants to merge 5 commits into
kernelci:mainfrom
aliceinwire:kcidev

Conversation

@aliceinwire

Copy link
Copy Markdown
Member

No description provided.

Signed-off-by: Arisu Tachibana <alicef@gentoo.org>
full-sized CLI command button with safe asynchronous clipboard handling

Signed-off-by: Arisu Tachibana <alicef@gentoo.org>
@aliceinwire

Copy link
Copy Markdown
Member Author

example:
image

Signed-off-by: Arisu Tachibana <alicef@gentoo.org>
Signed-off-by: Arisu Tachibana <alicef@gentoo.org>
Signed-off-by: Arisu Tachibana <alicef@gentoo.org>
@aliceinwire

Copy link
Copy Markdown
Member Author

Made kci-dev more visible

  • Moved kci-dev information to the Menu
  • Added a CLI command in the upper part
image

<div className="flex flex-wrap justify-end gap-2">
{kcidevComponent}
</div>
<div className="flex flex-wrap justify-end gap-2"></div>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

empty div

@alanpeixinho alanpeixinho left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice work @aliceinwire. Left a few comments.

testPaths: enabledValues(
result === 'boots' ? record.bootPath : record.testPath,
),
bootOrigins: enabledValues(record.bootOrigin),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Dont we need built and test origin filters as well?

<FormattedMessage id="footer.cliCommand" />
</Button>
</PopoverTrigger>
<PopoverContent

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think we can present this information in a more concise way, especially by using icons in the buttons.
Also, the install kci-dev command is always present, so we could take it off the Popover.

Image

This is just a proposal, there might be better ways to present the popover.

};

export type KcidevCommandVariant = {
id: 'human' | 'json' | 'download' | 'gate' | 'gate-json';

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

do we really need to include variants of commands?
If it is important to include every variant, cant we just ad d a radio (or another way to select), so we dont show of them at the start.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Image

issueCulprits?: readonly string[];
issueCategories?: readonly string[];
issueOptions?: readonly string[];
hasExactDateBoundaries?: boolean;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

are we using this parameter?

];

const appendOption = (argv: string[], option: string, value?: string): void => {
if (value !== undefined) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

might be safe to filter out empty string as well

omittedFilters: [...new Set(omittedFilters)],
reproduction: omittedFilters.length === 0 ? 'exact' : 'partial',
};
result.variants = variants ?? standardVariants(argv);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: cant we just initialize variants with the object initialization here

id: 'download',
label: 'Download logs (writes files)',
argv: [...argv, '--download-logs'],
writesFiles: true,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

is it being used?

label: string;
argv: readonly string[];
omittedFilters: readonly string[];
reproduction: 'exact' | 'partial';

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this prop being read?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants