Skip to content

Conversation

@glekner
Copy link
Contributor

@glekner glekner commented Nov 11, 2020

No description provided.

@openshift-ci-robot openshift-ci-robot added the component/kubevirt Related to kubevirt-plugin label Nov 11, 2020
@glekner glekner marked this pull request as draft November 11, 2020 17:33
@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 11, 2020
@glekner glekner changed the title Add VM Create to Dev console WIP: Add VM Create to Dev console Nov 11, 2020
@glekner glekner force-pushed the dev-console-add-vm branch 5 times, most recently from ea6e74d to cdac38d Compare November 12, 2020 11:14
@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 15, 2020
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 15, 2020
@glekner glekner force-pushed the dev-console-add-vm branch 8 times, most recently from 09b9cde to 94187cd Compare November 19, 2020 15:44
@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 19, 2020
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 20, 2020
@glekner glekner changed the title WIP: Add VM Create to Dev console Add VM Create to Dev console Nov 20, 2020
@glekner glekner marked this pull request as ready for review November 20, 2020 10:33
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 20, 2020
@glekner glekner force-pushed the dev-console-add-vm branch 2 times, most recently from 8c6f079 to d412cf1 Compare November 22, 2020 09:59
@glekner glekner force-pushed the dev-console-add-vm branch 4 times, most recently from 4df114b to ce8ca45 Compare November 30, 2020 13:24
@jelkosz
Copy link

jelkosz commented Nov 30, 2020

/hold cancel
design got approved, unholding

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 30, 2020
Copy link
Contributor

@rohitkrai03 rohitkrai03 Nov 30, 2020

Choose a reason for hiding this comment

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

Yeah, this does affect all the other providers. It would be better if we remove the spacing from the providers along with this change. Although looking at the code it seems only useBuilderImages provider added extra spacing by using <hr/> which can be removed here. All other providers use PF components or SyncMarkdown component that adds the spacing for them. Not sure what we can do about them. cc: @christianvogt

FYI, the above change adds extra spacing with current catalog providers as shown below-

Screenshot from 2020-11-30 21-54-35
Screenshot from 2020-11-30 21-55-24
Screenshot from 2020-11-30 21-56-58

@glekner glekner force-pushed the dev-console-add-vm branch 2 times, most recently from ce8ca45 to 7cdc11f Compare December 1, 2020 09:47
@glekner
Copy link
Contributor Author

glekner commented Dec 1, 2020

/retest

@glekner glekner force-pushed the dev-console-add-vm branch 3 times, most recently from ed5f908 to efcc1e3 Compare December 1, 2020 14:25
@glekner
Copy link
Contributor Author

glekner commented Dec 1, 2020

@christianvogt @rawagner @suomiy please lgtm

@spadgett spadgett removed their request for review December 1, 2020 14:38
@christianvogt
Copy link
Contributor

I'm fine with the removal of the <hr> in the builder image screen but that's also a @openshift/team-devconsole-ux call
(screenshot: https://user-images.githubusercontent.com/6041994/100635998-3936dc00-3357-11eb-9ea5-869d823c8679.png)

Copy link
Contributor

@christianvogt christianvogt left a comment

Choose a reason for hiding this comment

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

works well.
Couple minor comments.

Copy link
Contributor

Choose a reason for hiding this comment

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

To respect the user's preference, do not include the view type. This will bring the user to either the graph or list list view. Unless of course UX wants to force the user to the graph.

Suggested change
`/topology/ns/${getNamespace(vm)}?view=graph&selectId=${getUID(
`/topology/ns/${getNamespace(vm)}?selectId=${getUID(

Comment on lines 539 to 540
Copy link
Contributor

Choose a reason for hiding this comment

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

localize

@glekner glekner force-pushed the dev-console-add-vm branch from efcc1e3 to 911088a Compare December 2, 2020 09:58
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be one translation

Suggested change
{t('kubevirt-plugin~Virtual machine template')}: <strong>{templateParam}</strong>{' '}
{t('kubevirt-plugin~Virtual machine template {{ templateParam }} not found', { templateParam })}

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
label="Virtual Machine Template"
label={t('Virtual Machine Template')}

Copy link
Contributor

Choose a reason for hiding this comment

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

missing i18n

Copy link
Contributor

Choose a reason for hiding this comment

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

missing i18n

Copy link
Contributor

Choose a reason for hiding this comment

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

missing i18n

Copy link
Contributor

Choose a reason for hiding this comment

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

rebase error - i18n was removed

Copy link
Contributor

Choose a reason for hiding this comment

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

rebase error - i18n was removed

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// t('Create a Virtual Machine from a template')
// t('kubevirt-plugin~Create a Virtual Machine from a template')

@openshift-ci-robot openshift-ci-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 2, 2020
@glekner glekner force-pushed the dev-console-add-vm branch from 911088a to d570e2b Compare December 2, 2020 10:51
@glekner glekner force-pushed the dev-console-add-vm branch from d570e2b to adc3e03 Compare December 2, 2020 10:53
@rawagner
Copy link
Contributor

rawagner commented Dec 2, 2020

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 2, 2020
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: glekner, rawagner

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 2, 2020
@openshift-merge-robot openshift-merge-robot merged commit 2b520d1 into openshift:master Dec 2, 2020
@spadgett spadgett added this to the v4.7 milestone Dec 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. component/core Related to console core functionality component/dev-console Related to dev-console component/kubevirt Related to kubevirt-plugin component/sdk Related to console-plugin-sdk lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.