Add 'Create Bucket' form (via S3-compat API) #1608
Conversation
alfonsomthd
commented
Oct 3, 2024
alfonsomthd
commented
alfonsomthd
requested review from
GowthamShanmugam,
SanjalKatiyar and
bipuladh
packages/odf/components/s3-browser/create-bucket/CreateBucketForm.tsx
Outdated
Show resolved
Hide resolved
packages/odf/components/s3-browser/create-bucket/CreateBucketForm.tsx
Outdated
Show resolved
Hide resolved
packages/odf/components/s3-browser/create-bucket/CreateBucketForm.tsx
Outdated
Show resolved
Hide resolved
packages/odf/components/s3-browser/create-bucket/CreateBucketForm.tsx
Outdated
Show resolved
Hide resolved
| <div className="col-xs-1 co-empty__header" /> | ||
| </div> | ||
| {pairElems} | ||
| {hideHeaderWhenNoItems && _.isEmpty(pairElems) ? ( |
There was a problem hiding this comment.
we are already passing hideHeaderWhenNoItems, why not add _.isEmpty(pairElems) condition is the same as well ??
const hideHeaderWhenNoItems = _.isEmpty(pairElems) && OTHER_CONDITIONS; and pass hideHeaderWhenNoItems as a prop.
There was a problem hiding this comment.
_.isEmpty(pairElems) will only execute anyway when hideHeaderWhenNoItems is true...
There was a problem hiding this comment.
^ optional comment...
There was a problem hiding this comment.
_.isEmpty(pairElems) will only execute anyway when hideHeaderWhenNoItems is true...
That's intended: The intention is not to alter the current behavior: so i added a new flag (hideHeaderWhenNoItems) for the new behavior and isEmpty is the condition to check on the new behavior: it's been tested and it works as intended.
|
Isn't the selected tile supposed to get expanded (with blue line on bottom) ?? Here looks like when "S3" option is selected, tile is shrinking instead, do we need some CSS fix ?? |
The behavior is the PF one: |
alfonsomthd
force-pushed
the
bucket-create
branch
from
9d783fb
to
9c221d5
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alfonsomthd, SanjalKatiyar 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 |
openshift-merge-bot
bot
merged commit 2c5dfc0
into
red-hat-storage:master
| @@ -1,5 +1,7 @@ | |||
| import * as React from 'react'; | |||
| import { CreateOBC } from '@odf/core/components/mcg/CreateObjectBucketClaim'; | |||
| import CreateBucketForm from '@odf/core/components/s3-browser/create-bucket/CreateBucketForm'; | |||
There was a problem hiding this comment.
no issues in this import, it just looks very longer than './create-bucket/CreateBucketForm';
it just need an import from its own directory
There was a problem hiding this comment.
i can see all the files have following similar way of import, so its ok to skip this comment
There was a problem hiding this comment.
IMHO we should favor absolute imports (vscode intellisense does it) vs relative imports due to the confusion created by the relative path hell ('../../../../../../../../../../../oh-my.ts')
| @@ -1,5 +1,7 @@ | |||
| import * as React from 'react'; | |||
| import { CreateOBC } from '@odf/core/components/mcg/CreateObjectBucketClaim'; | |||
| import CreateBucketForm from '@odf/core/components/s3-browser/create-bucket/CreateBucketForm'; | |||
| import { NoobaaS3Provider } from '@odf/core/components/s3-browser/noobaa-context'; | |||
