Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Selects the default class or the first item in SnapshotClassDropdown #7050

Merged
merged 1 commit into from Jan 5, 2021

Conversation

vbnrh
Copy link
Contributor

@vbnrh vbnrh commented Oct 30, 2020

@openshift-ci-robot openshift-ci-robot added the component/core Related to console core functionality label Oct 30, 2020
@vbnrh vbnrh changed the title Adds a prop to list-dropdown to select the first item in the list Adds a prop to SnapshotClassDropdown to select the first item in the list Oct 30, 2020
@vbnrh
Copy link
Contributor Author

vbnrh commented Oct 30, 2020

@afreen23 @a2batic @bipuladh Please test and review.

@vbnrh
Copy link
Contributor Author

vbnrh commented Oct 30, 2020

/assign @afreen23 @a2batic @bipuladh

Copy link
Contributor

@afreen23 afreen23 left a comment

Choose a reason for hiding this comment

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

The ListDropdown is used at several places, are we sure we want to change the behavior for all ?
Moreover, you can use the selectedKey option to pass a pre selected item.

@cloudbehl
Copy link
Contributor

The ListDropdown is used at several places, are we sure we want to change the behavior for all ?
Moreover, you can use the selectedKey option to pass a pre selected item.

I agree we can use selectedKey and pass the 0th element of an array if we don't have default classes.

@vbnrh
Copy link
Contributor Author

vbnrh commented Nov 2, 2020

@cloudbehl @afreen23 Could you guys let me know on how we could use the selectedKey to use the 0th element of an array ? As per my understanding, the selectedKey needs a string value from an item in the array. We don't have the arrays available as a return value for using the first element in the create-volume-snapshot.tsx and passing it as selected key to listdropdown. Hence, i passed a prop from SnapshotClassDropdown to the listdropdown and select whatever is the first value there.

The ListDropdown is used at several places, are we sure we want to change the behavior for all

Which is why i introduced a prop, we are only changing the behavior of SnapshotClassDropdown.

@afreen23
Copy link
Contributor

afreen23 commented Nov 2, 2020

@cloudbehl @afreen23 Could you guys let me know on how we could use the selectedKey to use the 0th element of an array ? As per my understanding, the selectedKey needs a string value from an item in the array. We don't have the arrays available as a return value for using the first element in the create-volume-snapshot.tsx and passing it as selected key to listdropdown. Hence, i passed a prop from SnapshotClassDropdown to the listdropdown and select whatever is the first value there.

The ListDropdown is used at several places, are we sure we want to change the behavior for all

Which is why i introduced a prop, we are only changing the behavior of SnapshotClassDropdown.

Firstly, Pr without description/jira/BZs are very hard to follow.
And checking on the current behavior, we do show the "select snapshot class" placeholder,
Screenshot from 2020-11-02 15-08-49
So why do we want to pre-select a random snapshot class ?

Thanks!

@vbnrh
Copy link
Contributor Author

vbnrh commented Nov 2, 2020

@cloudbehl @afreen23 Could you guys let me know on how we could use the selectedKey to use the 0th element of an array ? As per my understanding, the selectedKey needs a string value from an item in the array. We don't have the arrays available as a return value for using the first element in the create-volume-snapshot.tsx and passing it as selected key to listdropdown. Hence, i passed a prop from SnapshotClassDropdown to the listdropdown and select whatever is the first value there.

The ListDropdown is used at several places, are we sure we want to change the behavior for all

Which is why i introduced a prop, we are only changing the behavior of SnapshotClassDropdown.

Firstly, Pr without description/jira/BZs are very hard to follow.
And checking on the current behavior, we do show the "select snapshot class" placeholder,
Screenshot from 2020-11-02 15-08-49
So why do we want to pre-select a random snapshot class ?

Thanks!

@afreen23 Sorry ! forgetting to link the JIRAs i am addressing.
This is to address https://issues.redhat.com/browse/RHSTOR-1345

We don't want to show "Select volume snapshot class" instead we'd like to already pre-select the default or the first item in the list.

@a2batic
Copy link
Contributor

a2batic commented Nov 2, 2020

So why do we want to pre-select a random snapshot class ?

@afreen23 It was suggested by UX team to pre-select snapshot class from the dropdown as an enhancement. The dropdown contains filtered options, so selecting any key from dropdown should work.
@yuvalgalanti ^^

Copy link
Contributor

@afreen23 afreen23 left a comment

Choose a reason for hiding this comment

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

So why do we want to pre-select a random snapshot class ?

@afreen23 It was suggested by UX team to pre-select snapshot class from the dropdown as an enhancement. The dropdown contains filtered options, so selecting any key from dropdown should work.
@yuvalgalanti ^^

It makes sense to pass the default storage class (if present) as selected from your parent component and let the user choose then any snapshot class.

@yuvalgalanti
Copy link

So why do we want to pre-select a random snapshot class ?

@afreen23 It was suggested by UX team to pre-select snapshot class from the dropdown as an enhancement. The dropdown contains filtered options, so selecting any key from dropdown should work.
@yuvalgalanti ^^

It makes sense to pass the default storage class (if present) as selected from your parent component and let the user choose then any snapshot class.

@afreen23 +1 If we are able to choose it by default we should do that.

@vbnrh vbnrh changed the title Adds a prop to SnapshotClassDropdown to select the first item in the list [WIP] Adds a prop to SnapshotClassDropdown to select the first item in the list Dec 9, 2020
@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 Dec 9, 2020
@vbnrh vbnrh force-pushed the pre-selected-vsc branch 5 times, most recently from 897ba60 to 2991683 Compare December 10, 2020 15:21
@vbnrh vbnrh changed the title [WIP] Adds a prop to SnapshotClassDropdown to select the first item in the list Selects the default class or the first item in SnapshotClassDropdown Dec 10, 2020
@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 Dec 10, 2020
Copy link
Contributor

@cloudbehl cloudbehl left a comment

Choose a reason for hiding this comment

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

define types for the variable declared

@vbnrh vbnrh force-pushed the pre-selected-vsc branch 2 times, most recently from 65e7871 to d8ac9f9 Compare December 14, 2020 09:20
Copy link
Contributor

@cloudbehl cloudbehl left a comment

Choose a reason for hiding this comment

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

nit

@a2batic
Copy link
Contributor

a2batic commented Dec 18, 2020

/lgtm
Working fine, tested locally.

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 18, 2020
@vbnrh vbnrh closed this Jan 4, 2021
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jan 4, 2021
@vbnrh vbnrh reopened this Jan 4, 2021
Signed-off-by: Vineet Badrinath <vbadrina@redhat.com>
@rawagner
Copy link
Contributor

rawagner commented Jan 5, 2021

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 5, 2021
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: a2batic, rawagner, vbnrh

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

The pull request process is described here

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 Jan 5, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jan 5, 2021

@vbnrh: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/prow/kubevirt-plugin 950e1d5 link /test kubevirt-plugin

Full PR test history. Your PR dashboard.

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.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit 34564ca into openshift:master Jan 5, 2021
@spadgett spadgett added this to the v4.7 milestone Jan 7, 2021
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 lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet