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.

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

refactor: refactor hooks useEventBroker and useLayer #1060

Merged
merged 5 commits into from
Oct 30, 2023
Merged

refactor: refactor hooks useEventBroker and useLayer #1060

merged 5 commits into from
Oct 30, 2023

Conversation

NikitaCG
Copy link
Contributor

Hook useEventBroker and useLayer very connected with their classes, therefore, there was no point in taking them out separately. Instead, I refactored folder for these utilities

@NikitaCG NikitaCG changed the title chore: hooks with appropriate class : useEventBroker , useLayer chore: reafactor hooks with appropriate class : useEventBroker , useLayer Oct 17, 2023
@gravity-ui-bot
Copy link
Contributor

Preview is ready.

korvin89
korvin89 reviewed Oct 24, 2023
View reviewed changes
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.

About naming: we are trying to use kebab-case for folders and files except react component files naming that uses camel case. Could u rename utils/eventBroker -> utils/event-broker?

@NikitaCG NikitaCG force-pushed the chore/hooks-with-appropriate-class branch from 51ccbb8 to 17553c7 Compare October 24, 2023 14:28
@NikitaCG
Copy link
Contributor Author

About naming: we are trying to use kebab-case for folders and files except react component files naming that uses camel case. Could u rename utils/eventBroker -> utils/event-broker?

done

amje
amje requested changes Oct 26, 2023
View reviewed changes
src/components/utils/layer-manager/index.ts Outdated
export type {
LayerConfig,
LayerExtendableProps,
ContentElement,
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if we want LayerExtendableProps and ContentElement to be exposed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@amje amje mentioned this pull request Oct 27, 2023
Closed
@NikitaCG NikitaCG force-pushed the chore/hooks-with-appropriate-class branch from a7281d4 to a71a795 Compare October 30, 2023 08:12
@amje amje changed the title chore: reafactor hooks with appropriate class : useEventBroker , useLayer refactor: reafactor hooks useEventBroker and useLayer Oct 30, 2023
@amje amje changed the title refactor: reafactor hooks useEventBroker and useLayer refactor: refactor hooks useEventBroker and useLayer Oct 30, 2023
@NikitaCG NikitaCG merged commit 3b07947 into main Oct 30, 2023
3 checks passed
@NikitaCG NikitaCG deleted the chore/hooks-with-appropriate-class branch October 30, 2023 14:58
@NikitaCG NikitaCG mentioned this pull request Nov 3, 2023
Closed
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

@korvin89 korvin89 korvin89 approved these changes

@LakeVostok LakeVostok Awaiting requested review from LakeVostok

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

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@NikitaCG @gravity-ui-bot @amje @korvin89