Add buckets overview page (header, details/objects list tabs, actions) layout #1591

SanjalKatiyar · 2024-09-20T09:59:32Z

https://issues.redhat.com/browse/RHSTOR-6196
https://issues.redhat.com/browse/RHSTOR-6199

packages/odf/components/s3-browser/bucket-overview/BucketOverview.tsx

packages/odf/utils/s3-browser.ts

packages/shared/src/hooks/useRefresh.ts

alfonsomthd · 2024-09-26T12:32:16Z

@SanjalKatiyar you should rebase in order to fix conflicts inlocales/en/plugin__odf-console.json.

SanjalKatiyar · 2024-09-26T12:33:59Z

@SanjalKatiyar you should rebase in order to fix conflicts inlocales/en/plugin__odf-console.json.

Ack, will do once PR is reviewed and fix it along with other review comments.

GowthamShanmugam · 2024-09-27T12:58:09Z

packages/odf/components/s3-browser/bucket-overview/BucketOverview.tsx

+  ...(isCreatedByOBC
+    ? [
+        {
+          title: t('Edit labels'),
+          onClick: () =>
+            launcher(defaultModalMap[ModalKeys.EDIT_LABELS], {
+              extraProps: {
+                resource: noobaaObjectBucket,
+                resourceModel: NooBaaObjectBucketModel,
+              },
+              isOpen: true,
+            }),
+        },
+      ]
+    : []),
+  ...(isCreatedByOBC
+    ? [
+        {
+          title: t('Edit annotations'),
+          onClick: () =>
+            launcher(defaultModalMap[ModalKeys.EDIT_ANN], {
+              extraProps: {
+                resource: noobaaObjectBucket,
+                resourceModel: NooBaaObjectBucketModel,
+              },
+              isOpen: true,
+            }),
+        },
+      ]
+    : []),
+  ...(isCreatedByOBC
+    ? [
+        {
+          title: t('Edit bucket'),
+          onClick: () => navigate(`${BUCKETS_BASE_ROUTE}/${bucketName}/yaml`),
+        },
+      ]


is there any reason the same check has been repeated? we can move three options from the same list, right?

will update...

GowthamShanmugam · 2024-09-27T13:11:04Z

packages/odf/components/s3-browser/bucket-overview/BucketOverview.tsx

+          },
+        ]
+      : []),
+    ...(isCreatedByOBC


is YAML visible for folder view?

ok! isCreatedByOBC will be false for folder view, got it
I prefer to move comment (will be false if we are inside folder view): https://github.com/red-hat-storage/odf-console/pull/1591/files#diff-1f39da47c37a872f0a045b71647ab27ae7d8e5527e3c38457b910e79538ef615R118 here

GowthamShanmugam · 2024-09-27T13:22:48Z

packages/odf/components/s3-browser/bucket-overview/BucketOverview.tsx

+      )
+    );
+  const noobaaObjectBucket: K8sResourceKind = objectBuckets?.find(
+    (ob) => ob.spec?.endpoint?.bucketName === bucketName


The doubt is do we need to loop all object buckets? we can use the bucketName in useK8sWatchResource to look for the specific object right? if the bucket name is found then it is OBC then not found then s3.

"ObjectBucket" CR name is different from actual underlying "Bucket" name... CR's spec will have the correct name...

GowthamShanmugam · 2024-09-27T13:32:51Z

packages/odf/components/s3-browser/bucket-overview/PageTitle.tsx

+  const { t } = useCustomTranslation();
+
+  const { noobaaS3 } = React.useContext(NoobaaS3Context);
+  const { data, error, isLoading } = useSWR(LIST_BUCKET, () =>


ok! there is no API endpoint to read a specific bucket?

yeah, I did not find any...

@SanjalKatiyar have you cheked HeadBucket ?

Nevermind, it seems it doesn't contain the info you need...

GowthamShanmugam · 2024-09-27T13:40:16Z

packages/odf/components/s3-browser/bucket-overview/PageTitle.tsx

+    <CreatedOnSkeleton />
+  ) : (
+    <h4 className="text-muted">
+      {t('Created on: ') + bucketCreatedOn?.toString()}


OCP console follows UTC date-time format, we need to makesure it display on a same format

I am keeping it as it is for now... ODF shows UTC only for tooltip, we don't have tooltip right now here... other issue is Timestamp FC display time as eg: 7 mins ago etc which works well for websocket connections as time gets continuously updated, but here we have to manually refresh...

IIUC displayed format here will be dependent on local timezone... we can confirm on @alfonsomthd system once this PR is merged...

@SanjalKatiyar the UK dev cluster has set the London timezone, so we can test it there.

need to check, in case SDK passes the timezone info to the server based off browser's settings then even UK cluster will not work for the verification... u will have to later run specifically on your system and verify...

GowthamShanmugam · 2024-09-27T13:46:29Z

packages/odf/components/s3-browser/bucket-overview/PageTitle.tsx

+  return (
+    <div className="pf-v5-u-display-flex pf-v5-u-flex-direction-column">
+      <div className="pf-v5-u-display-flex pf-v5-u-flex-direction-row">
+        {!foldersPath ? bucketName : currentFolder}{' '}


Suggested change

{!foldersPath ? bucketName : currentFolder}{' '}

{!foldersPath ? bucketName : currentFolder}

I think it was added by linter, will check again though...

GowthamShanmugam · 2024-09-27T13:57:50Z

packages/odf/components/s3-browser/bucket-overview/PageTitle.tsx

+      </div>
+      {!foldersPath &&
+        (fresh ? <CreatedOn bucketName={bucketName} /> : <CreatedOnSkeleton />)}
+      <h4>


just asking myself is correct to add components inside tags like h, Text, p, etc..
will figure it out :)

yeah, no issues with that...

GowthamShanmugam · 2024-09-27T14:01:21Z

packages/odf/components/s3-browser/objects-list/ObjectsList.tsx

+  CustomActionsToggleProps,
+} from '@patternfly/react-table';
+import { LIST_OBJECTS, DELIMITER, MAX_KEYS, PREFIX } from '../../../constants';
+import { ObjectCrFormat } from '../../../types';


Suggested change

import { ObjectCrFormat } from '../../../types';

import { ObjectCRFormat } from '../../../types';

thoughts?

either is fine IMO, we don't follow any specific rule in this repo... I have seen both naming formats...

GowthamShanmugam · 2024-09-27T14:01:42Z

packages/odf/components/s3-browser/objects-list/ObjectsList.tsx

+} from '@patternfly/react-table';
+import { LIST_OBJECTS, DELIMITER, MAX_KEYS, PREFIX } from '../../../constants';
+import { ObjectCrFormat } from '../../../types';
+import { getPath, convertObjectsDataToCrFormat } from '../../../utils';


Suggested change

import { getPath, convertObjectsDataToCrFormat } from '../../../utils';

import { getPath, convertObjectsDataToCRFormat } from '../../../utils';

same here

GowthamShanmugam · 2024-09-27T14:14:09Z

packages/odf/components/s3-browser/objects-list/ObjectsList.tsx

+    if (isNext) {
+      newTokens.previous.push(newTokens.current);
+      newTokens.current = newTokens.next;
+    } else {
+      newTokens.current = newTokens.previous.pop();
+    }
+    newTokens.next = response.NextContinuationToken;


Suggested change

if (isNext) {

newTokens.previous.push(newTokens.current);

newTokens.current = newTokens.next;

} else {

newTokens.current = newTokens.previous.pop();

}

newTokens.next = response.NextContinuationToken;

if (isNext) {

newTokens.previous.push(newTokens.current);

newTokens.current = newTokens.next;

newTokens.next = response.NextContinuationToken;

} else {

// I am assuming this is for back

newTokens.next = newTokens.current

newTokens.current = newTokens.previous.pop();

}

because back noting to do with response.NextContinuationToken I think

current conditions in the PR are tested and works well...

Ok! it works becuase in esponse.NextContinuationToken and newTokens.current may have same values.

But i agree it is safer to use in response.NextContinuationToken for newTokens.next. because,if any object is deleted, then newTokens.current is not the right value to use. i like this logic.

packages/odf/components/s3-browser/bucket-overview/BucketOverview.tsx

bipuladh · 2024-09-30T07:43:29Z

packages/odf/components/s3-browser/bucket-overview/PageTitle.tsx

+  <Skeleton width="25%" height="15%" />
+);
+
+const CreatedOn: React.FC<{ bucketName: string }> = ({ bucketName }) => {


I didn't get it, I am using type, right ?? just I am not storing it in a separate variable as it's only a single prop...

We always use a different variable in our codebase. So it would be nice to stick to the conventions.

GowthamShanmugam · 2024-09-30T08:28:06Z

packages/odf/components/s3-browser/objects-list/table-components.tsx

+  const name = object.metadata.name.replace(foldersPath, '');
+  const prefix = !!foldersPath
+    ? encodeURIComponent(foldersPath + name)
+    : encodeURIComponent(name);


prefix is required for non-foldersPath also? means, when the foldersPath is empty then encodeURIComponent for prefix and prefix used under {isFolder ? (

os isFolder can be true when foldersPath is empty ?

this is used for creating a URL for the folder view, each folder view will have a prefix in the URL...

initial buckets page will not have foldersPath, but when we drill down prefix will be folder name...

GowthamShanmugam · 2024-09-30T08:29:03Z

packages/odf/components/s3-browser/objects-list/table-components.tsx

+
+  const { launcher, bucketName, foldersPath } = extraProps;
+  const isFolder = object.isFolder;
+  const name = object.metadata.name.replace(foldersPath, '');


Suggested change

const name = object.metadata.name.replace(foldersPath, '');

const name = getName(object).replace(foldersPath, '');

optional

GowthamShanmugam · 2024-09-30T08:29:46Z

packages/odf/components/s3-browser/objects-list/table-components.tsx

+      <Td translate={null} dataLabel={columnNames[3]}>
+        {object.apiResponse.lastModified}
+      </Td>
+      <Td translate={null} dataLabel={columnNames[3]} isActionCell>


columnNames[3] is used twice, is this expected?

@GowthamShanmugam @SanjalKatiyar this is why I would prefer for all our tables to not depend on hardcoded column numbers but iterating and using the iterator index variable (or column id) so adding/removing a column should be error-free.
Here is how I did it for storage client list.

@SanjalKatiyar not saying that my suggestion should be applied here in this PR: just something that we should discuss (also better alternatives) for future implementations.

From the link above, what I understood is, that you are suggesting to use name as a column id.

…) layout

bipuladh · 2024-09-30T16:43:58Z

packages/odf/components/s3-browser/bucket-overview/BucketOverview.tsx

+  const actions = () => {
+    return (


Suggested change

const actions = () => {

return (

const actions = () => (

nit

bipuladh · 2024-09-30T16:44:47Z

packages/odf/components/s3-browser/bucket-overview/BucketOverview.tsx

+          variant={ButtonVariant.link}
+          icon={<BlueSyncIcon />}
+          onClick={triggerRefresh}
+          isDisabled={!fresh}


Does fresh mean being refreshed? If that is the case let's rename it to isRefreshing

fresh = true means it's latest (mounting/mounted)... fresh = false means it's refreshing (un-mounting/un-mounted)...

bipuladh · 2024-09-30T16:45:32Z

packages/odf/components/s3-browser/bucket-overview/PageTitle.tsx

+  <Skeleton width="25%" height="15%" />
+);
+
+const CreatedOn: React.FC<{ bucketName: string }> = ({ bucketName }) => {


We always use a different variable in our codebase. So it would be nice to stick to the conventions.

bipuladh · 2024-09-30T16:46:13Z

packages/odf/components/s3-browser/bucket-overview/PageTitle.tsx

+  );
+};
+
+const BucketResourceStatus: React.FC<{ resourceStatus: string }> = ({


Same as above. Let's stick to our conventions.

I see multiple such instance where we are using types directly for a FC (instead of a variable), even I wasn't aware that we have such a convention :P

bipuladh · 2024-09-30T16:49:35Z

packages/odf/components/s3-browser/bucket-overview/bucket-overview.scss

+}
+
+.bucket-label--margin-top {
+    margin-top: calc(0.3rem * -1);


why are we using negative margin to pull things? There should be a better way to do this.

Just curious, is something wrong with -ve values ??
Even MDN describe it as A positive value places it farther from its neighbors, while a negative value places it closer in it's very first paragraph :/ (also, I didn't find it being mentioned as a bad practice or bad convention).

bipuladh · 2024-09-30T16:53:58Z

/lgtm
We can take up the minor comments in the follow up PRs

openshift-ci · 2024-09-30T16:54:05Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bipuladh, 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:

~~OWNERS~~ [SanjalKatiyar,bipuladh]

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

openshift-ci bot added do-not-merge/work-in-progress approved labels Sep 20, 2024

SanjalKatiyar force-pushed the s3_object_list branch 8 times, most recently from 82f6556 to 4caedcc Compare September 25, 2024 14:30

SanjalKatiyar changed the title ~~[WIP] Add buckets overview page (header, details/objects list tabs, actions) layout~~ Add buckets overview page (header, details/objects list tabs, actions) layout Sep 25, 2024

openshift-ci bot removed the do-not-merge/work-in-progress label Sep 25, 2024

alfonsomthd reviewed Sep 25, 2024

View reviewed changes

packages/odf/components/s3-browser/bucket-overview/BucketOverview.tsx Outdated Show resolved Hide resolved

alfonsomthd reviewed Sep 25, 2024

View reviewed changes

packages/odf/utils/s3-browser.ts Outdated Show resolved Hide resolved

alfonsomthd reviewed Sep 25, 2024

View reviewed changes

packages/shared/src/hooks/useRefresh.ts Show resolved Hide resolved

openshift-merge-robot added the needs-rebase label Sep 26, 2024

GowthamShanmugam mentioned this pull request Sep 26, 2024

RHSTOR-6195: Bucket - list page #1598

Merged

GowthamShanmugam reviewed Sep 27, 2024

View reviewed changes

bipuladh reviewed Sep 30, 2024

View reviewed changes

GowthamShanmugam reviewed Sep 30, 2024

View reviewed changes

SanjalKatiyar force-pushed the s3_object_list branch from 4caedcc to 4ce0c35 Compare September 30, 2024 10:33

openshift-merge-robot removed the needs-rebase label Sep 30, 2024

SanjalKatiyar requested review from alfonsomthd, GowthamShanmugam and bipuladh September 30, 2024 10:35

Add buckets overview page (header, details/objects list tabs, actions…

baab291

…) layout

SanjalKatiyar force-pushed the s3_object_list branch from 4ce0c35 to baab291 Compare September 30, 2024 10:53

bipuladh reviewed Sep 30, 2024

View reviewed changes

openshift-ci bot assigned bipuladh Sep 30, 2024

openshift-ci bot added the lgtm label Sep 30, 2024

openshift-merge-bot bot merged commit eb8baf6 into red-hat-storage:master Sep 30, 2024
5 checks passed

	{!foldersPath ? bucketName : currentFolder}{' '}
	{!foldersPath ? bucketName : currentFolder}

	import { ObjectCrFormat } from '../../../types';
	import { ObjectCRFormat } from '../../../types';

	import { getPath, convertObjectsDataToCrFormat } from '../../../utils';
	import { getPath, convertObjectsDataToCRFormat } from '../../../utils';

	const name = object.metadata.name.replace(foldersPath, '');
	const name = getName(object).replace(foldersPath, '');

Add buckets overview page (header, details/objects list tabs, actions) layout #1591

Add buckets overview page (header, details/objects list tabs, actions) layout #1591

Conversation

SanjalKatiyar commented Sep 20, 2024 • edited Loading

alfonsomthd commented Sep 26, 2024

SanjalKatiyar commented Sep 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GowthamShanmugam Sep 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GowthamShanmugam Sep 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SanjalKatiyar Sep 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GowthamShanmugam Sep 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alfonsomthd Sep 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SanjalKatiyar Sep 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SanjalKatiyar Sep 30, 2024 • edited Loading

Choose a reason for hiding this comment

bipuladh commented Sep 30, 2024

openshift-ci bot commented Sep 30, 2024

SanjalKatiyar commented Sep 20, 2024 •

edited

Loading

GowthamShanmugam Sep 27, 2024 •

edited

Loading

GowthamShanmugam Sep 27, 2024 •

edited

Loading

SanjalKatiyar Sep 30, 2024 •

edited

Loading

GowthamShanmugam Sep 27, 2024 •

edited

Loading

alfonsomthd Sep 30, 2024 •

edited

Loading

SanjalKatiyar Sep 30, 2024 •

edited

Loading

SanjalKatiyar Sep 30, 2024 •

edited

Loading