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

Fixed backingstore creation page css and added basic check. #3213

Merged
merged 1 commit into from Nov 15, 2019

Conversation

bipuladh
Copy link
Contributor

@bipuladh bipuladh commented Nov 4, 2019

  • Fixed the info popover to span the full length of the screen
  • Added basic check for target container and backing store name.
    Scrshot:
    Screenshot from 2019-11-04 18-05-28

@openshift-ci-robot openshift-ci-robot added component/noobaa Related to noobaa-storage-plugin size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 4, 2019
const [disabled, setDisabled] = React.useState(true);

React.useEffect(() => {
if (bsName.length > 0) setDisabled(false);
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
if (bsName.length > 0) setDisabled(false);
if (bsName.length > 0 || providerDataState.target.length > 0) setDisabled(false);
else setDisabled(true);

Copy link
Contributor Author

@bipuladh bipuladh Nov 4, 2019

Choose a reason for hiding this comment

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

&& ?

@bipuladh bipuladh force-pushed the fix_basic_sanity branch 2 times, most recently from 6beb20c to c130d20 Compare November 4, 2019 13:11
@cloudbehl
Copy link
Contributor

/kind bug

@openshift-ci-robot openshift-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Nov 4, 2019
@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 11, 2019
@cloudbehl
Copy link
Contributor

/test e2e-gcp-console
/lgtm
/approve

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 13, 2019
@bipuladh
Copy link
Contributor Author

/hold

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 13, 2019
@@ -58,6 +58,7 @@ const CreateBackingStoreFormPage: React.FC<CreateBackingStoreFormPageProps> = ({
variant="info"
title="What is a BackingStore?"
action={<AlertActionCloseButton onClose={() => setShowHelp(false)} />}
className="nb-bs__info"
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this class for ?
Not seeing used anywhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

right! removed it during merge conflicts. But forgot to remove this. Thanks.

Comment on lines 487 to 489
React.useEffect(() => {
if (bsName.length > 0 && providerDataState.target.length > 0) setDisabled(false);
else setDisabled(true);
}, [bsName, providerDataState]);
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the use case for setting it through state, why not using isRequired ?
The button itself wont submit without it and will display a message.
This would reduce extra costs on UI end too.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, are we allowing user to create backing store without selecting secret ?
I think secret should be selected as required too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are scenarios when secret is not required. Hence no need to enforce it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

isRequired does not disable the button. It adds the *. But the request still goes through.

Copy link
Contributor

@afreen23 afreen23 Nov 13, 2019

Choose a reason for hiding this comment

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

yeah! and you will get error message to fill out the field.
I feel it should be like that and this is how it is at various places in openshift.
e.g create pvc, projects,etc.

@afreen23
Copy link
Contributor

I am seeing this info for backing store inconsistent with that of bucket class' info.
There is no Learn More icon here also line spacings are different.
We should make it as one component and reuse it.
This will make the info consistent at all places.

<Alert
isInline
variant="info"
title="What is a BackingStore?"
action={<AlertActionCloseButton onClose={() => setShowHelp(false)} />}
className="nb-bs__info"
>
BackingStore represent a storage target to be used as the underlying storage for the data
in MCG buckets.
<br />
Multiple types of backing-stores are supported: aws-s3, s3-compataible,
google-cloud-storage, azure-blob, obc, PVC.
</Alert>

@bipuladh
Copy link
Contributor Author

bipuladh commented Nov 13, 2019

I am seeing this info for backing store inconsistent with that of bucket class' info.
There is no Learn More icon here also line spacings are different.
We should make it as one component and reuse it.
This will make the info consistent at all places.

<Alert
isInline
variant="info"
title="What is a BackingStore?"
action={<AlertActionCloseButton onClose={() => setShowHelp(false)} />}
className="nb-bs__info"
>
BackingStore represent a storage target to be used as the underlying storage for the data
in MCG buckets.
<br />
Multiple types of backing-stores are supported: aws-s3, s3-compataible,
google-cloud-storage, azure-blob, obc, PVC.
</Alert>

This is how the design is. There is no external link. @afreen23

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

const { cancel, close, inProgress, errorMessage, handlePromise, isPage } = props;
React.useEffect(() => {
if (bsName.length > 0 && providerDataState.target.length > 0) setDisabled(false);
Copy link
Contributor

Choose a reason for hiding this comment

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

dont you need to call trim() to make sure you wont accept just space like ?

You can also make it shorter by calling just setDisabled(!(bsName.length > 0 && providerDataState.target.length > 0))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ack

@afreen23
Copy link
Contributor

afreen23 commented Nov 13, 2019

I am seeing this info for backing store inconsistent with that of bucket class' info.
There is no Learn More icon here also line spacings are different.
We should make it as one component and reuse it.
This will make the info consistent at all places.

<Alert
isInline
variant="info"
title="What is a BackingStore?"
action={<AlertActionCloseButton onClose={() => setShowHelp(false)} />}
className="nb-bs__info"
>
BackingStore represent a storage target to be used as the underlying storage for the data
in MCG buckets.
<br />
Multiple types of backing-stores are supported: aws-s3, s3-compataible,
google-cloud-storage, azure-blob, obc, PVC.
</Alert>

This is how the design is. There is no external link. @afreen23

I just feel info at both places would be consistent. It could be missed though @shirimordechay ^^

@bipuladh bipuladh force-pushed the fix_basic_sanity branch 2 times, most recently from c42c86b to d01178a Compare November 13, 2019 13:00
@cloudbehl
Copy link
Contributor

/test e2e-gcp-console

@bipuladh bipuladh force-pushed the fix_basic_sanity branch 2 times, most recently from 11f22c4 to 2e31aac Compare November 13, 2019 15:12
@bipuladh
Copy link
Contributor Author

/hold cancel

@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 14, 2019
@bipuladh
Copy link
Contributor Author

/retest images e2e-gcp-console

@afreen23
Copy link
Contributor

/retest

@openshift-ci-robot openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 14, 2019
@afreen23
Copy link
Contributor

/lgtm

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: afreen23, bipuladh, cloudbehl

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-bot
Copy link
Contributor

/retest

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

@afreen23
Copy link
Contributor

/retest

@openshift-bot
Copy link
Contributor

/retest

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

4 similar comments
@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@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 c4a84a6 into openshift:master Nov 15, 2019
@bipuladh bipuladh deleted the fix_basic_sanity branch November 15, 2019 06:22
@spadgett spadgett added this to the v4.3 milestone Nov 19, 2019
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/noobaa Related to noobaa-storage-plugin kind/bug Categorizes issue or PR as related to a bug. 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
Development

Successfully merging this pull request may close these issues.

None yet

8 participants