Skip to content

feat(Gallery): add Gallery component #247

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Mar 26, 2025
Merged

feat(Gallery): add Gallery component #247

merged 1 commit into from
Mar 26, 2025

Conversation

kseniya57
Copy link
Contributor

Add the components for rendering galleries (base gallery and the files gallery with file previews and some pre-defined active file renderers)

@kseniya57 kseniya57 self-assigned this Dec 15, 2024
@gravity-ui-bot
Copy link
Contributor

Preview is ready.

mournfulCoroner
mournfulCoroner requested changes Dec 16, 2024
View reviewed changes
mournfulCoroner
mournfulCoroner reviewed Dec 16, 2024
View reviewed changes
Copy link
Contributor

@mournfulCoroner mournfulCoroner left a comment •
edited
Loading

Choose a reason for hiding this comment

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

It's impossible to interact with video after its panel is gone. Hover does not work and click switches the file. Is that how it's supposed to work?

@korvin89
Copy link
Contributor

@kseniya57 Do we have rfc fo this components?

@kseniya57
Copy link
Contributor Author

@kseniya57

Yes

@kseniya57 kseniya57 force-pushed the gallery branch 3 times, most recently from 401a5d6 to 8c07955 Compare December 19, 2024 05:43
@kseniya57 kseniya57 force-pushed the gallery branch 2 times, most recently from 855b632 to 772a07f Compare December 26, 2024 07:54
@kseniya57 kseniya57 changed the title feat(Gallery): add Gallery and FilesGallery components feat(Gallery): add Gallery component Dec 26, 2024
@mournfulCoroner mournfulCoroner removed the request for review from KirillDyachkovskiy January 15, 2025 15:31
mournfulCoroner
mournfulCoroner reviewed Jan 16, 2025
View reviewed changes
@mournfulCoroner
Copy link
Contributor

Storybook tests fail. Please try to update the branch

@kseniya57
Copy link
Contributor Author

Storybook tests fail. Please try to update the branch

The branch is already up to date, so seems like the problem is in the main or in the ci. I will try to rerun, but I also did that a week ago.

@kseniya57 kseniya57 force-pushed the gallery branch 2 times, most recently from 513124e to 070ce7f Compare January 29, 2025 13:44
mournfulCoroner
mournfulCoroner reviewed Feb 5, 2025
View reviewed changes
@kseniya57 kseniya57 force-pushed the gallery branch 2 times, most recently from 78cb8b3 to 9bf19d6 Compare February 10, 2025 15:48
amje
amje requested changes Mar 5, 2025
View reviewed changes
src/components/Gallery/Gallery.scss Outdated
}
}

#{$filePreviewBlock} {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it able to not modify the internal styles of another component? It's not safe

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need the new compact view for that component to avoid this

Copy link
Contributor

Choose a reason for hiding this comment

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

Relying on class names is not safe, especially from another package, because these names are "private" data, they could change in any minor update. The best solution here is to add flexibility to the FilePreview either through props or CSS API.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created a pull request: gravity-ui/uikit#2157

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Used the compact view property and removed this custom styles

src/components/Gallery/Gallery.scss Outdated
.g-root_theme_light,
.g-root_theme_light-hc {
#{$block}__header {
background-color: var(--g-color-private-white-450);
Copy link
Contributor

Choose a reason for hiding this comment

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

Private variables are not for the usage in components. It should be changed to semantic variables, which already have colors in both light/dark themes

Copy link
Contributor Author

@kseniya57 kseniya57 Mar 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I looked through all the colors and didn't find a suitable one, the thing is that this color should be black transparent in a dark theme and light transparent in a light one. Also the color should match the background of the gallery, but be transparent.

Copy link
Contributor

Choose a reason for hiding this comment

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

Private variables are potential subject of change in future minor releases, the color might change or even the variable itself might be deleted. If these colors needs to be added to the list of semantic color they should be validated with the design team. Or you can use plain colors here

src/components/Gallery/hooks/useNavigation.ts Outdated
export type UseNavigationProps = {
itemsCount: number;
initialItemIndex?: number;
selectedPreviewItemClass: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

It's better use refs list for targeting DOM node. We can replace two props itemsCount and selectedPreviewItemClass with itemRefs: React.Ref[]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Isn't this a more resource-intensive approach? So you need to create a ref for each element, and there can be many elements.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am sure it's not. Refs are the React way of manipulation DOM

@kseniya57 kseniya57 requested a review from amje March 5, 2025 18:57
amje
amje requested changes Mar 11, 2025
View reviewed changes
src/components/Gallery/__stories__/Gallery.stories.tsx Outdated
const ImagesGalleryTemplate: StoryFn<GalleryProps> = () => {
const [open, setOpen] = React.useState(false);

const container = usePortalContainer();
Copy link
Contributor

Choose a reason for hiding this comment

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

Why specify container? usePortalContainer returns the default one anyway

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it was a bug fix, but I can't remember that bug, maybe it was related to the old uikit version

@kseniya57 kseniya57 requested a review from amje March 12, 2025 06:19
@kseniya57 kseniya57 merged commit 28b1ca7 into main Mar 26, 2025
4 checks passed
@kseniya57 kseniya57 deleted the gallery branch March 26, 2025 14:56
@gravity-ui-bot gravity-ui-bot mentioned this pull request Mar 24, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@amje amje amje approved these changes

@mournfulCoroner mournfulCoroner mournfulCoroner approved these changes

@ValeraS ValeraS Awaiting requested review from ValeraS ValeraS is a code owner

@korvin89 korvin89 Awaiting requested review from korvin89 korvin89 is a code owner

Assignees

@kseniya57 kseniya57

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@kseniya57 @gravity-ui-bot @korvin89 @mournfulCoroner @amje