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

feat: new component Sheet & PromoSheet #420

Merged
merged 1 commit into from
Dec 22, 2022
Merged

Conversation

Avol-V
Copy link
Contributor

@Avol-V Avol-V commented Dec 13, 2022

Add components Sheet (MobileModal) and PromoSheet.

Copy link
Contributor

@korvin89 korvin89 left a comment

Choose a reason for hiding this comment

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

You could add codeowners right here. Sheet - @korvin89 PromoSheet - @Avol-V

<Sheet
className={cn(null, className)}
contentClassName={cn('content', contentClassName)}
visible={visible && loaded}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we have a lag here in case of slow connection?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a promo-informer and should be visible only when it's ready to show. So it's just hidden while image loading and we don't need any visible loader.


| Property | Type | Required | Default | Description |
| :----------------------- | :--------- | :------: | :---------- | :-------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
| visible | `boolean` | ✓ | | Show/hide sheet |
Copy link
Contributor

Choose a reason for hiding this comment

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

Why it should be a required property?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was required in the original component. Maybe to highlight that you should to specify visible property to control visibility of the component, it's useless without visible property.

}

.yc-root {
--yc-sheet-content-paddings: 0 10px;
Copy link
Contributor

Choose a reason for hiding this comment

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

padding, in singular

export interface SheetProps {
onClose?: () => void;
/** Show/hide sheet */
visible: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

We use open for this

/** Enable the behavior in which you can close the sheet window with a swipe down if the content is scrolled to its top (`contentNode.scrollTop === 0`) or has no scroll at all */
allowHideOnContentScroll?: boolean;
/** Hide top bar with resize handle */
noTopBar?: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

hideTopBar

interface SheetContentBaseProps {
hideSheet: () => void;
content: React.ReactNode;
visible: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

open


interface SheetContentBaseProps {
hideSheet: () => void;
content: React.ReactNode;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why don't use children for it?

visible: boolean;
}

export class Sheet extends React.Component<SheetProps, SheetState> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why class component, not function?

private static bodyScrollLocksCount = 0;
private static bodyInitialOverflow: string | undefined = undefined;

static lockBodyScroll() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Its already implemented in useBodyScrollLock

@@ -0,0 +1,142 @@
import React from 'react';
import ReactDOM from 'react-dom';
import {SheetContentContainer} from './SheetContent';
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's name member as the filename

inWindowResizeScope: boolean;
}

class SheetContent extends React.Component<SheetContentInnerProps, SheetContentState> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can be also implemented as a function

Comment on lines +60 to +67
--yc-promo-sheet-margin: 8px;
--yc-promo-sheet-padding: 20px;
--yc-promo-sheet-border-radius: 12px;
--yc-promo-sheet-header-margin: 12px;
--yc-promo-sheet-message-margin: 16px;
--yc-promo-sheet-image-margin: 12px;
--yc-promo-sheet-foreground: var(--yc-color-text-light-primary);
--yc-promo-sheet-background: var(--yc-my-color-brand-normal);
Copy link
Contributor

Choose a reason for hiding this comment

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

Same question here, we would allow to style this component?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes

Copy link
Contributor

@amje amje left a comment

Choose a reason for hiding this comment

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

Components will be refactored in the future #435

@gravity-ui-bot
Copy link
Contributor

Preview is ready.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants