[de-xbl] remove badgebutton binding
Categories
(Thunderbird :: Instant Messaging, task)
Tracking
(Not tracked)
People
(Reporter: mkmelin, Assigned: khushil324)
References
Details
(Whiteboard: [Thunderbird-testfailure: Z all][Thunderbird-disabled-test])
Attachments
(1 file, 2 obsolete files)
|
(deleted),
patch
|
jorgk-bmo
:
review+
|
Details | Diff | Splinter Review |
The badgebutton binding looks to me like it's redundant.
It is mapped to one toobarbutton #button-chat
https://searchfox.org/comm-central/source/mail/components/im/content/badgebutton.xml#13
I think all the styling can be just done on the toolbar button.
The functionality of updating the label is only called in (maybe) on place, https://searchfox.org/comm-central/rev/47187196fbf02cc0d635371b3879ddf5a49f491c/mail/components/im/content/chat-messenger.js#325
|
Assignee | |
Comment 1•6 years ago
|
||
I think we should convert it to Custom Element, it will be fairly simple. What do you suggest?
|
Reporter | |
Comment 2•6 years ago
|
||
I think in this case it's not needed. None of the other toolbar buttons are custom elements. Since it's so little functionality, I'd check if we can't just handle this from the outside.
It should be just removing the binding, adjusting the css slightly, and then the setting of the count. Essentially.
|
|
Reporter | |
Comment 4•6 years ago
|
||
We should wait for bug 1538983 which will make it easier and more clear what to do.
|
|
Reporter | |
Comment 5•6 years ago
|
||
Bug 1519577 is the new thing to track. Bug 1538983 only converted normal buttons.
|
|
Reporter | |
Updated•6 years ago
|
|
||
Comment 6•5 years ago
|
||
I'll have to disable mozmill/im/test-toolbar-buttons.js | test-toolbar-buttons.js::test_toolbar_and_placeholder
Pushed by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/5ee712009254 temporarily disable test-toolbar-buttons.js::test_toolbar_and_placeholder. rs=bustage-fix
|
|
||
Updated•5 years ago
|
|
|
Assignee | |
Comment 9•5 years ago
|
||
|
|
Reporter | |
Comment 10•5 years ago
|
||
Comment on attachment 9067579 [details] [diff] [review] Bug-1538547_de-xbl_chat-badgebutton.patch Review of attachment 9067579 [details] [diff] [review]: ----------------------------------------------------------------- This doesn't atm use very similar structure as the MozToolbarbutton. Would it be possible to make it that? I mean, possibly even just let it add the fragment, and override render() to add our label? Maybe also just badgeCount as an observed attribute?
|
|
Reporter | |
Comment 11•5 years ago
|
||
Comment on attachment 9067579 [details] [diff] [review] Bug-1538547_de-xbl_chat-badgebutton.patch Review of attachment 9067579 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/components/im/content/toolbarbutton-badge-button.js @@ +20,5 @@ > + */ > + class MozBadgebutton extends customElements.get("toolbarbutton") { > + static get inheritedAttributes() { > + return { > + ...super.inheritedAttributes, for instance, these would not be working properly since there is too much changed structure compared to the parent
|
|
Assignee | |
Comment 12•5 years ago
|
||
(In reply to Magnus Melin [:mkmelin] from comment #10)
This doesn't atm use very similar structure as the MozToolbarbutton.
Would it be possible to make it that? I mean, possibly even just let it add
the fragment, and override render() to add our label?Maybe also just badgeCount as an observed attribute?
I don't think that it is possible like toolbarbutton-menu-button because we want a badge over the image. In toolbarbutton-menu-button, we are adding a dropmarker besides the toolbarbutton. We can have a badge over the whole toolbarbutton but it will not look good in terms of UI/UX.
|
|
Assignee | |
Comment 13•5 years ago
|
||
|
|
Assignee | |
Updated•5 years ago
|
|
|
||
Comment 14•5 years ago
|
||
Comment on attachment 9069648 [details] [diff] [review] Bug-1538547_de-xbl_chat-badgebutton.patch Sorry for stealing this. As far as I can see, this works and it's also like a cook-cutting exercise. It also fixes bug 1555136 which seems to come from some "refresh" issue due to a mix of XBL code removed here and the already de-XBL'ed other parts. The test passes, too, so I'll enable it.
|
||
Comment 15•5 years ago
|
||
Pushed by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/f6f0c189c55a [de-xbl] convert badgebutton binding to custom element. r=jorgk
|
|
||
Updated•5 years ago
|
|
|
||
Updated•5 years ago
|
|
|
Assignee | |
Updated•5 years ago
|
Description
•