-
Notifications
You must be signed in to change notification settings - Fork 975
Style ledger notifications like update notifications #3856
Conversation
sweet! thanks! |
i haven't looked into why, but this appears to be applying the style of the first notification bar to all the notification bars below it, regardless of whether they are ledger notifications. to repro:
if you close the first notification so that the geolocation notification is on top, then the ledger notification loses its style otherwise i feel it |
I initially tried to style each notification separately, but that looked really ugly due to variations in margin/padding. Alternatively we could sort and group notifications by style, and separate them with borders on top/bottom. |
i feel it even more now |
We need to sync up the style on the yellow alert. Can we use 3 buttons instead? No / yes / always Or similar....?
|
It would have to be four buttons (No, Yes, Always No, Always Yes) |
b8714f1
to
a0e50c5
Compare
@diracdeltas Notifications are now sorted by style and prioritize styled notifications over the defaults: @bradleyrichter Besides restyling the 2 ledger notifications, this doesn't affect other notifications so I think it's out of scope for this PR. |
a0e50c5
to
d00fe90
Compare
|
||
// Insert notification next to those with the same style, or at the end | ||
let insertIndex = notifications.size | ||
const style = action.detail.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.
for checking against immutable, you might check for the presence of toJS instead; I know a lot of other code does that (just a consistency nitpick)
Comments left on the last commit 👍 |
// Insert notification next to those with the same style, or at the end | ||
let insertIndex = notifications.size | ||
const style = action.detail.get | ||
? action.detail.get('options').get('style') |
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.
for future reference, you can just do thing.getIn(['options', 'style'])
to avoid it throwing an error if thing.get('options')
is falsy
this seems to cause some of the notification bar tests to fail. https://travis-ci.org/brave/browser-laptop/builds/159031606 otherwise lgtm, feel free to merge! |
d00fe90
to
636c466
Compare
636c466
to
b239577
Compare
@diracdeltas thx, i've updated navigationBar test to use the new css selector |
git rebase -i
to squash commits (if needed).Partially implements #3257
Auditors: @bradleyrichter @diracdeltas
Test Plan:
true
Preview: