Refactor/doma 10993/moved base news form into step #5723
Conversation
Alexander-Turkin
temporarily deployed
to
internal
GitHub Actions
Inactive
Seems like this commit should not be here |
| isValid: boolean | ||
| } | ||
|
|
||
| export type TScope = { |
There was a problem hiding this comment.
We use other syntax: TScope -> ScopeType
| @@ -143,59 +132,51 @@ type SelectAppsFormValues = { | |||
| validBefore: string | |||
| } | |||
|
|
|||
| const HiddenBlock = styled.div<{ hide?: boolean }>` | |||
| export type SharingAppValues = { | |||
There was a problem hiding this comment.
Why do you need it here? You have already declared this exact variable in InputStep
| export const NO_RESIZE_STYLE: React.CSSProperties = { resize: 'none' } | ||
| export const FLEX_START_STYLE: React.CSSProperties = { alignItems: 'flex-start' } | ||
| export const BIG_MARGIN_BOTTOM_STYLE: React.CSSProperties = { marginBottom: '60px' } | ||
| export const MARGIN_BOTTOM_32_STYLE: React.CSSProperties = { marginBottom: '32px' } | ||
| export const MARGIN_BOTTOM_38_STYLE: React.CSSProperties = { marginBottom: '38px' } | ||
| export const MARGIN_BOTTOM_10_STYLE: React.CSSProperties = { marginBottom: '10px' } | ||
| export const MARGIN_BOTTOM_24_STYLE: React.CSSProperties = { marginBottom: '24px' } | ||
| export const MARGIN_TOP_8_STYLE: React.CSSProperties = { marginTop: '8px' } | ||
| export const MARGIN_TOP_44_STYLE: React.CSSProperties = { marginTop: '44px' } | ||
| export const FORM_FILED_COL_PROPS = { style: { width: '100%', padding: 0, height: '44px' } } | ||
| export const SCROLL_TO_FIRST_ERROR_CONFIG: ScrollOptions = { behavior: 'smooth', block: 'center' } | ||
| export const SHOW_TIME_CONFIG = { defaultValue: dayjs('00:00:00:000', 'HH:mm:ss:SSS') } | ||
| export const FULL_WIDTH_STYLE: React.CSSProperties = { width: '100%' } | ||
| const SMALL_VERTICAL_GUTTER: [Gutter, Gutter] = [0, 24] | ||
| const EXTRA_SMALL_VERTICAL_GUTTER: [Gutter, Gutter] = [0, 10] | ||
| const BIG_HORIZONTAL_GUTTER: [Gutter, Gutter] = [50, 0] | ||
| const ALL_SQUARE_BRACKETS_OCCURRENCES_REGEX = /\[[^\]]*?\]/g | ||
| const ADDITIONAL_DISABLED_MINUTES_COUNT = 5 | ||
| const DATE_FORMAT = 'DD MMMM YYYY HH:mm' | ||
| export const SMALL_VERTICAL_GUTTER: [Gutter, Gutter] = [0, 24] | ||
| export const EXTRA_SMALL_VERTICAL_GUTTER: [Gutter, Gutter] = [0, 10] | ||
| export const BIG_HORIZONTAL_GUTTER: [Gutter, Gutter] = [50, 0] | ||
| export const ALL_SQUARE_BRACKETS_OCCURRENCES_REGEX = /\[[^\]]*?\]/g | ||
| export const ADDITIONAL_DISABLED_MINUTES_COUNT = 5 | ||
| export const DATE_FORMAT = 'DD MMMM YYYY HH:mm' |
There was a problem hiding this comment.
I do not really like exporting this stuff elsewhere,
It is considered a bad practice in this project, better to stick to UI kit where possible, and just inline required values
You are exporting these values, this means that other engineers will use them, and project might become more coupled (https://www.geeksforgeeks.org/software-engineering-coupling-and-cohesion/) + this promotes bad practice
Lets remove these exports
| const res = { ...form.getFieldsValue(true) } | ||
|
|
||
| setSelectedTitle(res.title) | ||
| setSelectedBody(res.body) | ||
| setCondoFormValues(res) |
| )} | ||
| </> | ||
| ) : ( | ||
| //TODO use custom selector with getRecipientCounter |
There was a problem hiding this comment.
If you add TODO, you should add a task number
| B2BApp, B2BAppNewsSharingConfig, NewsItem as INewsItem, | ||
| NewsItemScope as INewsItemScope, | ||
| Property as IProperty, | ||
| } from '../../../../schema' |
There was a problem hiding this comment.
You should use absolute imports when possible
| @@ -104,18 +92,19 @@ type StepData = { | |||
|
|
|||
| export type SendPeriodType = 'now' | 'later' | |||
|
|
|||
| export type TInitialValues = Partial<INewsItem> & Partial<{ | |||
| const { app: sharingApp, id: ctxId } = sharingAppData ?? { ctxId: null, sharingApp: null } | ||
| const { id, newsSharingConfig } = sharingApp ?? { id: null, newsSharingConfig: null } |
There was a problem hiding this comment.
I do not really like that we have one big component that renders both sharing app and not sharing app. Does not look clean to me
I'd like to propose something like this:
BaseNewsForm - handles steps and skips logic
- InputStep – just takes building blocks and compiles them in single component
-
- bodyControl/IFrameBodyControl.tsx
-
- bodyControl/CondoBodyControl.tsx
-
- scopeControl/CondoScopeControl.tsx
-
- scopeControl/GetRecipientsScopeControl.tsx
-
- RecipientsDisplay.tsx
-
- PreviewDisplay.tsx
We can discuss this
|
@Alexander-Turkin Better to use rebase always |
|
Also, beware, when merging this PR you will be squasing all your commits into one Name of squashed commit = name of the PR, so in your case name of the squashed commit will be: Refactor/doma 10993/moved base news form into step Best to use semantic commits syntax for that :-) |
Alexander-Turkin
temporarily deployed
to
internal
GitHub Actions
Inactive
Alexander-Turkin
temporarily deployed
to
internal
GitHub Actions
Inactive
| [key: string]: { | ||
| title: string | ||
| body: string | ||
| type: string | null |
There was a problem hiding this comment.
| type: string | null | |
| type?: string |
?
| import { TemplatesSelect } from '../../TemplatesSelect' | ||
| import { getBodyTemplateChangedRule, getTitleTemplateChangedRule } from '../BaseNewsForm' | ||
| import { TemplatesType } from '../BaseNewsForm' |
There was a problem hiding this comment.
| import { TemplatesSelect } from '../../TemplatesSelect' | |
| import { getBodyTemplateChangedRule, getTitleTemplateChangedRule } from '../BaseNewsForm' | |
| import { TemplatesType } from '../BaseNewsForm' | |
| import { TemplatesSelect } from '@condo/domains/news/components/TemplatesSelect' | |
| import { getBodyTemplateChangedRule, getTitleTemplateChangedRule, TemplatesType } from '@condo/domains/news/components/NewsForm/BaseNewsForm' |
use absolute import if it's other folder
| return style | ||
| } | ||
|
|
||
| interface InputStepFormProps { |
There was a problem hiding this comment.
15 props looks too complicated. Try to reduce their number (for example, break them into different components, if possible)
There was a problem hiding this comment.
here I take out a huge piece of code so that a large number
There was a problem hiding this comment.
I +1 @nomerdvadcatpyat, we can for example move onSkip to the BaseNewsForm, that would lower the complexity
| TitleInput: any | ||
| BodyInput: any |
There was a problem hiding this comment.
ReturnType<typeof useInputWithCounter> istead any
| newsItemData: { | ||
| type: string | ||
| validBefore?: string | ||
| title: string | ||
| body: string | ||
| } |
There was a problem hiding this comment.
Same type used in INewsItemSharingForm. Make separate type and reuse here and in INewsItemSharingForm
| } | ||
| selectedBody: string | ||
| selectedTitle: string | ||
| form: any |
There was a problem hiding this comment.
FormInstance from antd.
And try to use useForm hook instead pass it in props:
const [form] = Form.useForm()
| selectedBody: string | ||
| selectedTitle: string | ||
| form: any | ||
| ctxId: string |
There was a problem hiding this comment.
What is this context? It's not clear from the name
| isValid: boolean | ||
| } | ||
|
|
||
| handleTemplateChange: (form: any) => (value: any) => void |
| <> | ||
| { | ||
| isCustomForm ? ( | ||
| <Col style={{ marginLeft: '-10px', minHeight: '500px' }} span={formFieldsColSpan}> |
There was a problem hiding this comment.
I copied this code)
There was a problem hiding this comment.
Let me explain this one: since customForm is an IFrame, we should have negative margin, in order for IFrame to be able to render shadows, outlines, and other UI stuff.
Best to add a comment, so that is more obvious
| import { NEWS_SHARING_PUSH_NOTIFICATION_SETTINGS } from '@condo/domains/miniapp/constants' | ||
| import { NEWS_TYPE_EMERGENCY } from '@condo/domains/news/constants/newsTypes' | ||
|
|
||
| import { MemoizedCondoNewsPreview, MemoizedSharingNewsPreview } from '../../NewsPreview' |
There was a problem hiding this comment.
| import { MemoizedCondoNewsPreview, MemoizedSharingNewsPreview } from '../../NewsPreview' | |
| import { MemoizedCondoNewsPreview, MemoizedSharingNewsPreview } from '@condo/domains/news/components/NewsPreview' |
…wsForm-into-step' into refactor/DOMA-10993/moved-baseNewsForm-into-step
Alexander-Turkin
temporarily deployed
to
internal
GitHub Actions
Inactive
This reverts commit c6cf4de
Alexander-Turkin
temporarily deployed
to
internal
GitHub Actions
Inactive
This reverts commit 048f77e.
Alexander-Turkin
temporarily deployed
to
internal
GitHub Actions
Inactive
|
merged NewsItemSharingForm and BaseNewsForm into one component (Input Step)