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

OV-6: Loader component #14

Merged
merged 19 commits into from
Aug 20, 2024
Merged

OV-6: Loader component #14

merged 19 commits into from
Aug 20, 2024

Conversation

sergiy4
Copy link
Collaborator

@sergiy4 sergiy4 commented Aug 19, 2024

This PR adds Loader and LoaderOverlay components.
loader
loaderOverlay

@sergiy4 sergiy4 added the FE Fronted feature label Aug 19, 2024
@sergiy4 sergiy4 added this to the Release 1.0 milestone Aug 19, 2024
@sergiy4 sergiy4 self-assigned this Aug 19, 2024
@sergiy4 sergiy4 linked an issue Aug 19, 2024 that may be closed by this pull request
9 tasks
Copy link
Collaborator

Choose a reason for hiding this comment

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

use layout components for ui lib - https://v2.chakra-ui.com/docs/components/box

you can avoid some custom styles

Comment on lines 4 to 5
export { Loader } from './loader/loader.js';
export { LoaderOverlay } from './loader-overlay/loader-overlay.js';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please unite these 2 components

Copy link
Collaborator

Choose a reason for hiding this comment

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

Or pass Loader through props as children and leave it as Overlay

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I should export both of them.
image

Copy link
Collaborator

Choose a reason for hiding this comment

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

you can export both of them but there is no point in just importing Loader in LoaderOverlay component

Make it accept any component as children

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Okay, I've just done this.

@@ -0,0 +1,35 @@
import { Box, Circle, Flex, keyframes, Text } from '@chakra-ui/react';

const spin = keyframes`
Copy link
Collaborator

Choose a reason for hiding this comment

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

./contstants/spin-animation.ts

Copy link
Collaborator

Choose a reason for hiding this comment

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

also, you should add some transitions to overlay - https://v2.chakra-ui.com/docs/components/transitions

because it will cover all screen

Copy link
Collaborator

Choose a reason for hiding this comment

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

export Loader component as well

@@ -0,0 +1,8 @@
import { keyframes } from '@chakra-ui/react';

const spin = keyframes`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
const spin = keyframes`
const SPIN_ANIMATION = keyframes`


const LoaderOverlay = (): JSX.Element => {
return (
<Fade in={true}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does transition work if you hide overlay from the parent component?

animation={`${SPIN_ANIMATION} 1s linear infinite`}
></Circle>
</Box>
<Text fontSize="lg" marginTop="10px">
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we should use the shortcuts that chakra ui is providing us with (e.g. alignItems -> align, marginTop -> mt)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't like shortcasts because they are hard to read

@nikita-remeslov nikita-remeslov merged commit 1946223 into next Aug 20, 2024
2 checks passed
@nikita-remeslov nikita-remeslov deleted the task/OV-6-add-loader branch August 20, 2024 09:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FE Fronted feature
Projects
Status: To Be Tested
Development

Successfully merging this pull request may close these issues.

FEAT: Loader component
4 participants