feat: add FilePreview component #79

KirillDyachkovskiy · 2023-07-23T09:56:37Z

No description provided.

gravity-ui-bot · 2023-07-23T09:58:24Z

DarkGenius · 2023-07-24T09:37:00Z

src/components/FilePreview/FilePreview.tsx

+
+            URL.revokeObjectURL(createdUrl);
+        };
+    }, [file]);


Shouldn't there be previewSrc in dependencies list?

ogonkov · 2023-07-24T09:25:10Z

src/components/FilePreview/FilePreview.tsx

+        } catch (err) {}
+
+        return () => {
+            if (!createdUrl) return;


just return it inside try..catch

ogonkov · 2023-07-24T09:31:46Z

src/components/FilePreview/__tests__/FilePreview.test.tsx

+import {FilePreview} from '../FilePreview';
+import {CircleExclamation} from '@gravity-ui/icons';
+
+describe('FilePreview', () => {


I suggest to not to add wrappers for tests, this is already covered by Jest itself

ogonkov · 2023-07-24T09:36:51Z

src/components/FilePreview/README.md

+className,
+file,
+description,
+onClick,
+actions,


is it still needed?

ogonkov · 2023-07-24T09:37:28Z

src/components/FilePreview/FilePreview.tsx

+    actions?: FilePreviewActionProps[];
+}
+
+export const FilePreview: FC<FilePreviewProps> = ({


add diplayName please

Or use raw function instead, FC type has no advantages

src/components/FilePreview/FilePreviewAction.tsx

ogonkov · 2023-07-24T09:39:09Z

src/components/FilePreview/FilePreviewAction.tsx

+
+export const FilePreviewAction: FC<FilePreviewActionProps> = ({icon, title, href, onClick}) => {
+    const button = (
+        <Button


button must have aria-label, to be accessible

ogonkov · 2023-07-24T09:39:40Z

src/components/FilePreview/FilePreview.tsx

+                    </Tooltip>
+                )}
+            </Card>
+            <div className={cn('actions')}>


should it be rendered, if actions didn't exist?

ogonkov · 2023-07-24T09:41:16Z

src/components/FilePreview/utils.ts

@@ -0,0 +1,23 @@
+import {FileType} from './FilePreview';


may be move FileType to types.ts?

ogonkov · 2023-07-28T15:52:50Z

src/components/FilePreview/FilePreview.scss

+
+        padding: 4px 10px;
+
+        &-icon {


I'm personally prefer not to split Element name via & (OK if it is Modificator), but that not a hard rule

src/components/FilePreview/FilePreview.tsx

ogonkov · 2023-07-28T15:55:49Z

src/components/FilePreview/FilePreviewAction.tsx

+            href={href}
+            target="_blank"
+            size="s"
+            aria-label={title || 'FilePreview action'}


That should be explicitly set by user, it should not be omited

ogonkov · 2023-07-28T16:21:37Z

src/components/FilePreview/FilePreview.scss

+        position: absolute;
+        top: -10px;
+        right: -10px;
+        z-index: 10;


ogonkov · 2023-07-28T16:29:55Z

src/components/FilePreview/FilePreview.scss

+    &__actions {
+        position: absolute;
+        top: -10px;
+        right: -10px;


this 10px is because of padding 10px in __card?

ogonkov · 2023-07-28T16:31:43Z

src/components/FilePreview/README.md

+| Property | Type       | Required | Default | Description          |
+| -------- | ---------- | -------- | ------- | -------------------- |
+| icon     | `String`   | yes      |         | Action icon          |
+| title    | `String`   |          |         | Action hint on hover |


ogonkov · 2023-07-28T16:32:28Z

src/components/FilePreview/FilePreviewAction.tsx

+                href={href}
+                target="_blank"
+                size="s"
+                aria-label={title}


may be we should separate aria-label and tooltip text?

ogonkov · 2023-07-28T16:38:41Z

src/components/FilePreview/FilePreview.tsx

+            <Card className={cn('card')} view="clear" type="selection" onClick={onClick}>
+                {typeof imageSrc === 'string' ? (
+                    <div className={cn('card-image')}>
+                        <img className={cn('card-image-img')} src={imageSrc} alt={file.name} />


alt here is looks redundant, since we have visible file name

i suggest we should use aria-labelledby with id of visible text with filename

ogonkov · 2023-07-28T16:40:31Z

src/components/FilePreview/__tests__/FilePreview.test.tsx

+
+    const filePreview = screen.getByTestId(qaId);
+
+    expect(filePreview).toBeDefined();


this always will be true, since get* will fail when element is not present

ogonkov · 2023-07-28T16:43:05Z

src/components/FilePreview/__tests__/FilePreview.test.tsx

+    const filePreview = screen.getByText(fileName);
+
+    const user = userEvent.setup();
+    await act(() => user.click(filePreview));


why act?

usually it just

await userEvent.click(...) ``

ogonkov · 2023-07-28T16:44:42Z

src/components/FilePreview/__tests__/FilePreview.test.tsx

+        />,
+    );
+
+    const actions = screen.getAllByRole('button');


check by name

screen.getByRole('button', {name: 'some hint'});

ogonkov · 2023-08-03T11:59:14Z

src/components/FilePreview/FilePreview.tsx

@@ -0,0 +1,129 @@
+import React, {useEffect, useId, useState} from 'react';


useId usage is breaking change, this library is compatible up to react 16, as peerDependencies

ogonkov · 2023-08-03T12:01:48Z

src/components/FilePreview/FilePreview.tsx

+                        <Icon className={cn('icon-svg')} data={FILE_ICON[type]} size={20} />
+                    </div>
+                )}
+                <Tooltip id={`${id}-file-name`} content={file.name}>


this id is html attribute, that should be globally unique, i guess it's would be better to add some component prefix here like gc-file-preview-${id}-file-name

ogonkov · 2023-08-03T12:03:16Z

src/components/FilePreview/FilePreview.tsx

+                <div className={cn('actions')}>
+                    {actions.map((action, index) => (
+                        <FilePreviewAction
+                            key={`${id}-file-actions-${index}`}


id is not unique enough?

ogonkov · 2023-08-03T12:04:17Z

src/components/FilePreview/FilePreviewAction.tsx

+    onClick,
+}) => (
+    <Tooltip id={id} content={<Text variant="caption-2">{title}</Text>}>
+        {


this is required?

ogonkov · 2023-08-03T12:08:55Z

src/components/FilePreview/__tests__/FilePreview.test.tsx

+    );
+
+    const firstAction = screen.getByRole('button', {name: firstActionText});
+    expect(firstAction).toBeDefined();


this actually always be true, since get* throws when node could not be found

ogonkov · 2023-08-10T12:48:33Z

package.json

@@ -54,7 +54,7 @@
    "@gravity-ui/prettier-config": "^1.0.1",
    "@gravity-ui/stylelint-config": "^2.0.0",
    "@gravity-ui/tsconfig": "^1.0.0",
-    "@gravity-ui/uikit": "^5.0.0",


Update version in peers aswell

ogonkov · 2023-08-10T12:55:25Z

src/components/FilePreview/FilePreview.tsx

+    actions,
+}) => {
+    const defaultId = useUniqId();
+    const id = outerId ?? defaultId;


outerId is not required, id is for component itself, there is no need or user to override ids required for a11y

ogonkov · 2023-08-10T16:33:18Z

src/components/FilePreview/README.md

+| id       | `String`   |          |         | Action id               |
+| icon     | `String`   | ✓        |         | Action icon             |
+| title    | `String`   | ✓        |         | Action hint on hover    |
+| label    | `String`   |          | `title` | Action aria-label value |


you mean value of title prop?

ogonkov · 2023-08-10T16:40:00Z

src/components/FilePreview/FilePreviewAction.tsx

+}
+
+export const FilePreviewAction: FC<FilePreviewActionProps> = ({
+    id,


Generate this id in component, there is no point ti pass id that required only for a11y

src/components/FilePreview/FilePreviewAction.tsx

amje · 2023-08-11T15:04:46Z

src/components/FilePreview/FilePreview.tsx

+    actions?: FilePreviewActionProps[];
+}
+
+export const FilePreview: FC<FilePreviewProps> = ({


Or use raw function instead, FC type has no advantages

src/components/FilePreview/FilePreview.tsx

src/components/FilePreview/FilePreviewAction.tsx

src/components/FilePreview/FilePreview.tsx

src/components/FilePreview/index.ts

amje · 2023-08-15T10:52:46Z

src/components/FilePreview/FilePreview.scss

+#{$block} {
+    --gc-file-preview-box-shadow: none;
+    --gc-file-preview-border-radius: 4px;
+    --gc-file-preview-color-base-background: transparent;


Use "private" prefix here: --_--box-shadow. And don't use component name

amje · 2023-08-15T10:57:00Z

src/components/FilePreview/FilePreview.tsx

+                        <img
+                            className={cn('image-img')}
+                            src={previewSrc}
+                            aria-labelledby={`${id}-file-name`}


What is the purpose of this? img don't need an accessible name, it's not interactive. Besides, this id is not used anywhere else

amje · 2023-08-15T11:00:29Z

src/components/FilePreview/FilePreviewAction.tsx

+                view="raised"
+                pin="circle-circle"
+                href={href}
+                target="_blank"


Not sure if _blank should be the default.

amje · 2023-08-21T18:11:37Z

src/components/FilePreview/FilePreview.scss

+$smallRoundedButtonSize: 24px;
+
+#{$block} {
+    --gc-_-box-shadow: none;


no need in gc prefix these vars are local anyway. Let's follow the pattern --_--{name}, so it will be --_--box-shadow

amje · 2023-08-21T18:16:08Z

src/components/FilePreview/README.md

+| id       | `String`   |          |         | Action id               |
+| icon     | `String`   | ✓        |         | Action icon             |
+| title    | `String`   | ✓        |         | Action hint on hover    |
+| label    | `String`   |          | `title` | Action aria-label value |


We decided to remove this prop, and add extraProps

amje · 2023-08-21T18:18:04Z

src/components/FilePreview/__tests__/FilePreview.test.tsx

+
+import {FilePreview} from '../FilePreview';
+
+test('Renders base content', () => {


Let's wrap the tests into describe block to name test suite

KirillDyachkovskiy requested review from amje, ValeraS and korvin89 as code owners July 23, 2023 09:56

KirillDyachkovskiy changed the title ~~feat(FilePreview): add FilePreview component~~ feat: add FilePreview component Jul 23, 2023

DarkGenius reviewed Jul 24, 2023

View reviewed changes

ogonkov reviewed Jul 24, 2023

View reviewed changes

ogonkov reviewed Jul 28, 2023

View reviewed changes

KirillDyachkovskiy force-pushed the add-file-preview branch from 9fb73ae to 9019dcf Compare August 3, 2023 07:36

ogonkov requested changes Aug 3, 2023

View reviewed changes

ogonkov reviewed Aug 3, 2023

View reviewed changes

KirillDyachkovskiy and others added 6 commits August 9, 2023 17:56

feat(FilePreview): add FilePreview component

de080a9

feat(FilePreview): cleanup code

7235ef4

feat(FilePreview): convert enum to type

fdd2254

feat(FilePreview): make title required

949f209

feat: fix tests

2fc074e

feat: use useUniqId hook

9ad7428

KirillDyachkovskiy force-pushed the add-file-preview branch from 9019dcf to 9ad7428 Compare August 10, 2023 12:16

feat: fix test

8b81140

ogonkov reviewed Aug 10, 2023

View reviewed changes

KirillDyachkovskiy added 2 commits August 10, 2023 15:59

feat: remove magic number from styles

912042d

feat: remove id property

628a851

ogonkov reviewed Aug 10, 2023

View reviewed changes

ogonkov approved these changes Aug 10, 2023

View reviewed changes

amje requested changes Aug 11, 2023

View reviewed changes

KirillDyachkovskiy added 2 commits August 11, 2023 18:49

feat: fix minor issues

3a0eeab

feat: refused the Card component

ed4c3ee

amje requested changes Aug 15, 2023

View reviewed changes

feat: provide extra-props to button

dda267b

KirillDyachkovskiy changed the title ~~feat: add FilePreview component~~ fix: add FilePreview component Aug 21, 2023

KirillDyachkovskiy changed the title ~~fix: add FilePreview component~~ feat: add FilePreview component Aug 21, 2023

amje requested changes Aug 21, 2023

View reviewed changes

feat: fix readme

b2ab7cc

amje approved these changes Aug 23, 2023

View reviewed changes

Merge branch 'main' into add-file-preview

1e5b726

KirillDyachkovskiy merged commit c910ec5 into main Aug 23, 2023
3 checks passed

KirillDyachkovskiy deleted the add-file-preview branch August 23, 2023 12:40

gravity-ui-bot mentioned this pull request Aug 23, 2023

chore(main): release 2.4.0 #98

Merged


		const filePreview = screen.getByTestId(qaId);

		expect(filePreview).toBeDefined();

		@@ -0,0 +1,129 @@
		import React, {useEffect, useId, useState} from 'react';


		import {FilePreview} from '../FilePreview';

		test('Renders base content', () => {

feat: add FilePreview component #79

feat: add FilePreview component #79

Conversation

KirillDyachkovskiy commented Jul 23, 2023

gravity-ui-bot commented Jul 23, 2023

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

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

ogonkov Aug 10, 2023 • 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

ogonkov Aug 10, 2023 •

edited

Loading