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

feat(notification): allow configuration of default options #636

Merged
merged 20 commits into from
Oct 14, 2021
Merged

feat(notification): allow configuration of default options #636

merged 20 commits into from
Oct 14, 2021

Conversation

KarimovDev
Copy link
Contributor

@KarimovDev KarimovDev commented Aug 21, 2021 •
edited
Loading

PR Checklist

Please check if your PR fulfills the following requirements:

  • The commit message follows Conventional Commits
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Refactoring (no functional changes, no api changes)
  • Other... Please describe:

What is the current behavior?

Partially implements #133

What is the new behavior?

Default notification options can be set via a provider. This is a partial implementation of #133

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@KarimovDev KarimovDev force-pushed the feat-notification-alert-options branch 2 times, most recently from c28f3fe to d5f722c Compare August 21, 2021 21:56
waterplea
waterplea requested changes Sep 3, 2021
View reviewed changes
Copy link
Collaborator

@waterplea waterplea left a comment

Choose a reason for hiding this comment

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

Thank you for your help! Please check the comments and also reuse this token in Notification component for the inputs that overlap.

projects/core/modules/notifications/notification-alert/notification-alert-options.ts Outdated Show resolved Hide resolved
projects/core/modules/notifications/notification-alert/notification-alert-options.ts Outdated Show resolved Hide resolved
projects/core/modules/notifications/notification-alert/notification-alert.template.html Outdated Show resolved Hide resolved
@KarimovDev
Copy link
Contributor Author

Thank you for your help! Please check the comments and also reuse this token in Notification component for the inputs that overlap.

Done

@KarimovDev KarimovDev force-pushed the feat-notification-alert-options branch from f2c2669 to 5219349 Compare September 3, 2021 23:56
@KarimovDev KarimovDev changed the title feat(notification-alert): allow configuration of default options feat(notification): allow configuration of default options Sep 4, 2021
@KarimovDev KarimovDev force-pushed the feat-notification-alert-options branch from 1adb7b0 to b0e6078 Compare September 14, 2021 08:23
@lumberjack-bot
Copy link

lumberjack-bot bot commented Sep 14, 2021 •
edited
Loading

Pull request was closed ✔️

All saved screenshots (for current PR) were deleted 🗑️

waterplea
waterplea requested changes Sep 22, 2021
View reviewed changes
Copy link
Collaborator

@waterplea waterplea left a comment

Choose a reason for hiding this comment

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

@KarimovDev I'm coming back to this PR. Please see new comments :)

projects/core/modules/notifications/notification-alert/notification-alert.component.ts Outdated Show resolved Hide resolved
projects/core/modules/notifications/notification-alert/notification-alert.template.html Outdated Show resolved Hide resolved
projects/core/modules/notifications/notification-alert/notification-alert.template.html Outdated Show resolved Hide resolved
projects/core/modules/notifications/notification-alert/Notification-alert.ts Outdated Show resolved Hide resolved
@KarimovDev KarimovDev force-pushed the feat-notification-alert-options branch from 0206071 to 20cad2f Compare September 27, 2021 22:52
@github-actions
Copy link
Contributor

github-actions bot commented Sep 27, 2021 •
edited
Loading

Visit the preview URL for this PR (updated for commit 4e7d825):

https://taiga-ui--pr636-feat-notification-al-jmzva20s.web.app

(expires Wed, 29 Sep 2021 14:38:50 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

@KarimovDev KarimovDev force-pushed the feat-notification-alert-options branch from 20cad2f to 9a5c12a Compare September 27, 2021 23:03
@KarimovDev KarimovDev force-pushed the feat-notification-alert-options branch from 9a5c12a to bc270f5 Compare September 27, 2021 23:13
@KarimovDev KarimovDev force-pushed the feat-notification-alert-options branch from bc270f5 to 0d685ce Compare September 28, 2021 08:50
waterplea
waterplea requested changes Sep 28, 2021
View reviewed changes
Copy link
Collaborator

@waterplea waterplea left a comment

Choose a reason for hiding this comment

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

A couple small comments and let's go ahead and add (status: TuiNotification) => number | boolean to the autoClose type.

projects/core/modules/notifications/notification-alert/Notification-alert.ts Outdated Show resolved Hide resolved
projects/core/tokens/notification-options.ts Outdated Show resolved Hide resolved
@KarimovDev
Copy link
Contributor Author

KarimovDev commented Sep 28, 2021 •
edited
Loading

A couple small comments and let's go ahead and add (status: TuiNotification) => number | boolean to the autoClose type.

Done, with handler execution inside NotificationAlert class. @waterplea What do you think about moving notification-options to interfaces? In this case I can also create shared TuiNotificationAutoClose type and share it between token and input options

@KarimovDev
Copy link
Contributor Author

KarimovDev commented Oct 1, 2021 •
edited
Loading

A couple small comments and let's go ahead and add (status: TuiNotification) => number | boolean to the autoClose type.

Done, with handler execution inside NotificationAlert class. @waterplea What do you think about moving notification-options to interfaces? In this case I can also create shared TuiNotificationAutoClose type and share it between token and input options

@KarimovDev yeah, let's add a dedicated type to autoClose

@waterplea Done

@KarimovDev KarimovDev force-pushed the feat-notification-alert-options branch from 6e9dfe8 to 9301829 Compare October 2, 2021 17:38
@tinkoff-bot tinkoff-bot force-pushed the feat-notification-alert-options branch 3 times, most recently from 040be79 to ba2b0ef Compare October 7, 2021 12:33
@KarimovDev
Copy link
Contributor Author

@waterplea Should I do something else here? Some errors occur during the build process, but I can't figure out what is going on

@tinkoff-bot tinkoff-bot force-pushed the feat-notification-alert-options branch 2 times, most recently from 1904b37 to 0995735 Compare October 8, 2021 09:29
@tinkoff-bot tinkoff-bot force-pushed the feat-notification-alert-options branch from 0995735 to a5fe831 Compare October 8, 2021 12:10
@vladimirpotekhin vladimirpotekhin merged commit d89e523 into taiga-family:main Oct 14, 2021
@well-done-bot
Copy link

well-done-bot bot commented Oct 14, 2021

'Well done'

@vladimirpotekhin
Copy link
Member

@KarimovDev Thank you!

vladimirpotekhin added a commit that referenced this pull request Oct 14, 2021
@KarimovDev @vladimirpotekhin
feat(notification): allow configuration of default options (#636)
7803442 
* feat(notification-alert): allow configuration of default options

* fix(core): implement review notes

* fix(core): move notification interfaces, add autoClose type

* feat(notification-alert): allow configuration of default options

* fix(core): implement review notes

* fix(core): move notification interfaces, add autoClose type

* chore: resolve conflicts

* chore: fix build

Co-authored-by: Dima Karimov <d.karimov@aitarget.com>
Co-authored-by: Vladimir <vladimir.potekh@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@splincode splincode splincode left review comments

@vladimirpotekhin vladimirpotekhin vladimirpotekhin approved these changes

@waterplea waterplea waterplea approved these changes

@MarsiBarsi MarsiBarsi Awaiting requested review from MarsiBarsi

@nsbarsukov nsbarsukov Awaiting requested review from nsbarsukov

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@KarimovDev @waterplea @vladimirpotekhin @splincode