-
Notifications
You must be signed in to change notification settings - Fork 9
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(Notifications): static component implementation (DATAUI-1582) #57
Conversation
…DATAUI-1582-new-notifications-component
Preview is ready. |
…DATAUI-1582-new-notifications-component
…DATAUI-1582-new-notifications-component
@@ -0,0 +1,4 @@ | |||
export * from './definitions'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets add this component into /Notifications
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean moving the index.ts
's content into Notifications/index.ts
or moving the whole Notification
directory's content into Notifications
directory?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whole directory. I think /components
should have only exported elements. Notification
seems like part of Notifications
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/Notification
can be exported alone, if, for example, you already have some container that you want to use for your notifications that is different from what /Notifications
offers you
Another point is that /Notification
has it's own Notification.scss
. So if put together, the styles are going to be in one big file and become harder to read
Or is it a common practice in your repository to put different style files in one directory? If so, I don't see a problem putting them in a single directory
|
|
Yep, we do not have tests yet. And it is not good. U can add an issue for tests and do it later. I am talking about testing of basic functionality, for example here. There is a good reference for jest configuration here. |
After consulting our designer, I rewrote the |
…ications-component
That's how it looks
I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/?lang=en.