-
Notifications
You must be signed in to change notification settings - Fork 792
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): add button to mark all notifications as read #371
Conversation
lex111
commented
Sep 27, 2017
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.
Nice feature! Left some comments to review.
this.props.isPendingMarkAllNotificationsAsRead && | ||
!this.isLoading() | ||
) { | ||
this.getNotifications()(); |
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.
()() ?
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.
Because this function returns a function, it still needs to be called, so one more parentheses.
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.
How about rename getNotifications
as getFuncForGettingNotifications
, or something else?
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.
It is unlikely that the first name is fine (getFuncForGettingNotifications
), because this function does two things: not only returns another function, but also updates the notification counter.
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.
@lex111 Sorry, I didn't quite get your words. Can you tell me where "updates the notification counter"?
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.
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.
I think something like getNotificationsForCurrentTab
or something along those lines will be okay
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.
@lex111 I prefer to keep getNotifications
as simple as now, instead of mixing getNotificationsCount
in it.
Because there is another place calls this function, which just expects a function. After #360 is merged, I guess getNotificationsCount
will be called frequently by render
.
src/config/colors.js
Outdated
@@ -29,6 +29,8 @@ export const colors = { | |||
purple: '#8e44ad', | |||
orange: '#e67e22', | |||
githubDark: '#1f2327', | |||
mercury: '#e3e3e3', | |||
shark: '#24292e', | |||
}; |
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.
I know those are official names, but they're not explicite at all.
Also, do we really need to add news colors?
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.
As I understand it, we add all the colors to the configuration, or am I mistaken?
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.
yes, new colors should be added in configuration.
I was asking if we could nor reuse existing colors instead
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.
Okay, I remove those colors
src/locale/languages/en.js
Outdated
@@ -135,6 +135,7 @@ export const en = { | |||
allButton: 'All', | |||
retrievingMessage: 'Retrieving notifications', | |||
noneMessage: "You don't have any notifications of this type", | |||
markAllAsRead: 'Mark all notifications as read', |
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 sentence is too long for a button, + translations may be bigger.
Can we use "Mark all read" instead?
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.
The size of the button allows, so I made a detailed description, well I'll remove the word "notifications".
borderColor: colors.mercury, | ||
borderWidth: 1, | ||
borderRadius: 3, | ||
}, | ||
}); |
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.
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.
Hmm this is interesting... I agree that we should keep to a similar style in general, but I don't think a bulky button like this taking up a bunch of space is the best UI. I like how this style is more discrete at the top of the screen.
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.
Absolutely! Right after commenting on this, I started implementing a standard component to be used everywhere (small/normal/big, info/danger/..) :)
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.
Maybe yes, we can not make this button fixed, then it will be before the first notification, how's this?
In style, I think this is the best option, I mean that the button is stretched to full width.
150303e
to
4c233b1
Compare
@lex111 what about putting the button to the bottom of the screen? Makes more sense to me: I go to the tab, scroll down while watching my notifications, then decide to mark them all read |
It's rather controversial, I'm comfortable when it's on top, and that's why I made it position is fixed initially, so that it was always visible .. let's wait for the opinions of others. |
55067e7
to
acdb0f4
Compare
acdb0f4
to
4a95f12
Compare
I like the top, personally. It's more like what GitHub has. |
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.
Just a few points, and some merge conflicts popped up since we merged your other notifications screen changes :O Otherwise I'm loving this!
I also prefer it at the top 🙌
this.props.isPendingMarkAllNotificationsAsRead && | ||
!this.isLoading() | ||
) { | ||
this.getNotifications()(); |
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.
I think something like getNotificationsForCurrentTab
or something along those lines will be okay
color={colors.black} | ||
backgroundColor={colors.white} | ||
textStyle={styles.buttonGroupText} | ||
onPress={() => markAllNotificationsAsRead()} |
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.
Does this button show when unread
, pending
and all
tabs are selected? Not sure if that's the best way of doing things. Do you think it will make more sense to have it only show only when all
is selected (and then maybe having buttons that show Mark all unread notifications as read
and Mark all participating notifications as read
for the other 2 tabs?
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.
Even if you think we don't need the other two buttons in this PR, I think having the Mark ALL button in each of the tabs can be kind of misleading
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.
Currently, on notifications page this button is displayed everywhere, and it always marks notifications as read all notifications, i.e. there is no way to mark as read only participating notifications.
Let's display this button only on the first tab (unread)?
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.
Ah that's a very good point 🤔
Hmmmmmm, I think I'm okay with having it display only on the first tab in the case. If I'm not mistaken, all participating notifications are unread notifications anyway so this should be okay 👍
case 2: | ||
return all; | ||
default: | ||
return null; |
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.
Hey, looks like #notifications
method should always return an array, it is safer in default
return also an array( probably [])
I know that default cause should be not called, but I believe it's always nice to try keep the type return
consistency 💃
case 2: | ||
return all && isPendingAll; | ||
default: | ||
return null; |
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.
Same of #notifications
method, but this time I should return a boolean.
this.setState({ contentBlockHeight: height - sizes.tabBar }); | ||
}; | ||
|
||
keyExtractor = (item, index) => { |
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.
Hey, this is a pretty small piece of code, I think that could be added inline in the like that:
keyExtractor={(item, index) => index}
Ready for review. |
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.
Good stuff 🎉 🎉
Left a few observations here but let's get this merged in sooner rather than later 👍