-
Notifications
You must be signed in to change notification settings - Fork 48
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
Ability to change item tag #43
base: master
Are you sure you want to change the base?
Conversation
The wrong use of data as selector requires data attributes to be set in a manual way, instead using data parameter.
The correct use for the counter when there aren´t notifications is not display 0, is display empty value. Then you can automatically hide a badge using css rule `:empty`. Also you can set the number zero using `:empty:before { content: "0" }`
It's not clear to me why the encapsulating tag should be configurable on each message. Can you give an example how you use this? |
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.
Thank you for this PR @Eseperio, looks solid.
Left some comments about enhancements to be made 👍
assets/notifications.js
Outdated
|
||
html = $(self.opts.tag, { | ||
class: "notification notification-" + object['type'] + " "+(object.seen ? 'notification-seen' : 'notification-unseen'), | ||
"data-route":object['url'], |
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.
please use the object.url
form, with an extra space before 'object'
single quotes instead of double quotes
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.
Ok, i don´t know why i used brackets in that object. Is a mistake
@@ -205,6 +210,7 @@ public function registerAssets() | |||
'pollSeen' => !!$this->pollSeen, | |||
'pollInterval' => Html::encode($this->pollInterval), | |||
'counters' => $this->counters, | |||
'tag' => Html::beginTag($this->tag) | |||
]; |
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.
Instead of the user providing only the tag name, shouldn't we enable him to pass also attributes?
Maybe move the processing in the Javascript, I don't like that we're calling Yii just to get '<' '>'
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.
This is a little modification. I have seen that javascript template can be impoved in many ways. I have a few ideas, but now i´m busy with other projects. When i have time i´ll try to do it.
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.
Used .property instead [property]
With this modification user can change the name of the tag used to wrap the notification.
It is an optional parameter in order to keep BC.
Also there is another little tweak. When there aren´t notifications counters now displays nothing instead 0.
This allows the user hide a badge when there are not notifications by using
:empty
css property.If you need to keep 0 number then use css rule
.counterclass:empty:before { content: "0"}
The behavior of hiding a badge when it is empty is used by almost all admin templates.