Skip to content

disable the option to delete the last revision in traffic splitting modal - #3486

Merged
openshift-merge-robot merged 1 commit into
openshift:masterfrom
karthikjeeyar:traffic-splitting-modal
Nov 22, 2019
Merged

disable the option to delete the last revision in traffic splitting modal#3486
openshift-merge-robot merged 1 commit into
openshift:masterfrom
karthikjeeyar:traffic-splitting-modal

Conversation

@karthikjeeyar

@karthikjeeyar karthikjeeyar commented Nov 20, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor
  1. disables the option to delete row if we have only one revision in traffic splitting modal
  2. fixes the alignment of the Add revision button

disable_traffic_modal

Fixes: https://jira.coreos.com/browse/ODC-2250

@openshift-ci-robot openshift-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. component/knative Related to knative-plugin labels Nov 20, 2019
@karthikjeeyar

karthikjeeyar commented Nov 20, 2019

Copy link
Copy Markdown
Contributor Author

@openshift/team-devconsole-ux @serenamarie125 This PR needs your approval.

@karthikjeeyar

karthikjeeyar commented Nov 20, 2019

Copy link
Copy Markdown
Contributor Author

/assign @vikram-raj

serenamarie125
serenamarie125 suggested changes Nov 20, 2019
View reviewed changes

@serenamarie125 serenamarie125 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.

@karthikjeeyar rather than removing it in this case, please just disable the remove '-' icon.

This way upon adding additional revisions and deleting all but one revision, the icon will just change states rather than being added/removed.

@karthikjeeyar karthikjeeyar changed the title remove the option to delete the last revision in traffic splitting modal disable the option to delete the last revision in traffic splitting modal Nov 20, 2019
@karthikjeeyar
karthikjeeyar force-pushed the traffic-splitting-modal branch from d6cdc0b to adf1a4a Compare November 20, 2019 19:00
@openshift-ci-robot openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. component/dev-console Related to dev-console and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Nov 20, 2019
serenamarie125
serenamarie125 approved these changes Nov 20, 2019
View reviewed changes

@serenamarie125 serenamarie125 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.

LGTM

vikram-raj
vikram-raj reviewed Nov 20, 2019
View reviewed changes

@vikram-raj vikram-raj left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@karthikjeeyar disable - icon color is different here.

see the build config field disabled color.

Screenshot from 2019-11-21 04-41-06

Screenshot from 2019-11-21 04-48-41

@serenamarie125 Which one you recommend?

christianvogt
christianvogt reviewed Nov 21, 2019
View reviewed changes
Comment thread ...d/packages/dev-console/src/components/formik-fields/multi-column-field/MultiColumnField.scss Outdated

@christianvogt christianvogt Nov 21, 2019

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.

should be --pf-global--disabled-color--200 to align with the plain button variant.

I created a tech debt item to convert this delete button to an actual button.

@karthikjeeyar karthikjeeyar Nov 21, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

updated to --pf-global--disabled-color--200

@christianvogt christianvogt mentioned this pull request Nov 21, 2019
Closed
@karthikjeeyar
karthikjeeyar force-pushed the traffic-splitting-modal branch from adf1a4a to 81f2946 Compare November 21, 2019 06:51
vikram-raj
vikram-raj reviewed Nov 21, 2019
View reviewed changes

@vikram-raj vikram-raj left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 21, 2019
@karthikjeeyar
karthikjeeyar force-pushed the traffic-splitting-modal branch from 81f2946 to 195d093 Compare November 22, 2019 04:32
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Nov 22, 2019
christianvogt
christianvogt reviewed Nov 22, 2019
View reviewed changes
Comment thread ...kages/dev-console/src/components/formik-fields/multi-column-field/MultiColumnFieldFooter.tsx Outdated

@christianvogt christianvogt Nov 22, 2019

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.

Suggested change
style={{ padding: 0 }}
isInline

Comment thread ...packages/dev-console/src/components/formik-fields/multi-column-field/MultiColumnFieldRow.tsx Outdated

@christianvogt christianvogt Nov 22, 2019

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.

If you make toolTip the last param instead you can simplify to this, rather than duplicating the string Delete:

    minusCircleIcon(onDelete, disableDeleteRow)

@karthikjeeyar
karthikjeeyar force-pushed the traffic-splitting-modal branch from 195d093 to 029527a Compare November 22, 2019 04:50
@christianvogt

christianvogt commented Nov 22, 2019

Copy link
Copy Markdown
Contributor

/lgtm
/approve

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 22, 2019
@openshift-ci-robot

openshift-ci-robot commented Nov 22, 2019

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: christianvogt, karthikjeeyar, serenamarie125, vikram-raj

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 Nov 22, 2019
@openshift-merge-robot
openshift-merge-robot merged commit 1ec9f06 into openshift:master Nov 22, 2019
@openshift-ci-robot

openshift-ci-robot commented Nov 22, 2019

Copy link
Copy Markdown
Contributor

@karthikjeeyar: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
ci/prow/e2e-gcp-console 029527a link /test e2e-gcp-console

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@spadgett spadgett added this to the v4.3 milestone Nov 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@invincibleJai invincibleJai Awaiting requested review from invincibleJai
3 more reviewers
@vikram-raj vikram-raj vikram-raj left review comments
@christianvogt christianvogt christianvogt left review comments
@serenamarie125 serenamarie125 serenamarie125 approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@vikram-raj vikram-raj

@christianvogt christianvogt

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. component/dev-console Related to dev-console component/knative Related to knative-plugin lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Milestone

v4.3

Development

Successfully merging this pull request may close these issues.

7 participants

@karthikjeeyar @christianvogt @openshift-ci-robot @vikram-raj @serenamarie125 @spadgett @openshift-merge-robot