Skip to content

Conversation

sayres
Copy link
Contributor

@sayres sayres commented Apr 18, 2022

Please read and mark the following check list before creating a pull request:

Short description of what this resolves:

Modal and Popover components moved to RN's Modal component implementation

@sayres sayres requested a review from malashkevich April 18, 2022 14:16
@github-actions
Copy link

Try running it with Kitten Tricks

.

@github-actions
Copy link

Try running it with Kitten Tricks

.

@sayres sayres requested a review from malashkevich April 29, 2022 11:41
* };
* ```
*/
class ModalServiceType {
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this one as we are using react-native Modal?

visible?: boolean;
shouldUseContainer?: boolean;
children?: React.ReactNode;
animationType?: 'none' | 'slide' | 'fade';
Copy link
Member

Choose a reason for hiding this comment

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

Please extract to the type

animationType?: 'none' | 'slide' | 'fade';
onShow?: (event: NativeSyntheticEvent<any>) => void;
supportedOrientations?: Array<
'portrait' | 'portrait-upside-down' | 'landscape' | 'landscape-left' | 'landscape-right'
Copy link
Member

Choose a reason for hiding this comment

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

Please extract to the type

type PopoverModalProps = Overwrite<ModalProps, {
children?: React.ReactElement;
}>;
}> & Pick<ModalProps, 'animationType' | 'hardwareAccelerated' | 'supportedOrientations' | 'onShow'>;
Copy link
Member

Choose a reason for hiding this comment

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

Please extract to the type

return fitsBottom(frame, other) && fitsLeft(frame, other) && fitsRight(frame, other);
}
};
static INNER: PopoverPlacement = new class implements PopoverPlacement {
Copy link
Member

Choose a reason for hiding this comment

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

It does makes sense to split this file into multiple

Copy link
Member

@malashkevich malashkevich left a comment

Choose a reason for hiding this comment

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

A few comments, but looks good

@github-actions
Copy link

Try running it with Kitten Tricks

.

import { ThemeProviderProps } from '../theme/themeProvider.component';
import { ModalPanel } from '../modal/modalPanel.component';
// import { ModalPanel } from '../modal/modalPanel.component';
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't this you want to leave this comment here :)

@github-actions
Copy link

github-actions bot commented Nov 1, 2022

Try running it with Kitten Tricks

.

@sayres
Copy link
Contributor Author

sayres commented Nov 1, 2022

fixes #1585 #1553

# Conflicts:
#	package.json
#	src/components/devsupport/components/measure/measure.component.tsx
#	src/components/theme/application/applicationProvider.component.tsx
#	src/components/theme/modal/modal.service.tsx
#	src/components/theme/modal/modal.spec.tsx
#	src/components/theme/modal/modalPanel.component.tsx
#	src/components/ui/autocomplete/autocomplete.component.tsx
#	src/components/ui/autocomplete/autocomplete.spec.tsx
#	src/components/ui/datepicker/datepicker.spec.tsx
#	src/components/ui/datepicker/rangeDatepicker.spec.tsx
#	src/components/ui/menu/menuGroup.component.tsx
#	src/components/ui/modal/modal.component.tsx
#	src/components/ui/modal/modal.spec.tsx
#	src/components/ui/overflowMenu/overflowMenu.component.tsx
#	src/components/ui/popover/popover.component.tsx
#	src/components/ui/popover/popover.spec.tsx
#	src/components/ui/select/select.spec.tsx
#	src/components/ui/tab/tab.spec.tsx
#	src/components/ui/tooltip/tooltip.component.tsx
#	yarn.lock
@greenfrvr greenfrvr self-assigned this Dec 1, 2022
@greenfrvr greenfrvr merged commit c19ae7b into master Dec 1, 2022
@greenfrvr greenfrvr linked an issue Dec 7, 2022 that may be closed by this pull request
>
{this.props.anchor()}
</MeasureElement>
<View>

Choose a reason for hiding this comment

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

Why did you add this View component. It adds additional wrapper which broke some ui.

Choose a reason for hiding this comment

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

@sayres @malashkevich please check my comment

Choose a reason for hiding this comment

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

maybe there is should be Fragment ?

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.

vibration on Modal !?

6 participants