-
Notifications
You must be signed in to change notification settings - Fork 773
feat(notifications): add button to mark all notifications as read #371
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
Changes from 1 commit
5770590
28b763a
4c233b1
d30e356
1b96443
4a95f12
ea141e8
43dc3f0
c26eab9
e091f4d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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', | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This sentence is too long for a button, + translations may be bigger.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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". |
||
| }, | ||
| }, | ||
| search: { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,7 +13,7 @@ import { | |
| Image, | ||
| Platform, | ||
| } from 'react-native'; | ||
| import { ButtonGroup, Card, Icon } from 'react-native-elements'; | ||
| import { ButtonGroup, Card, Icon, Button } from 'react-native-elements'; | ||
|
|
||
| import { v3 } from 'api'; | ||
| import { | ||
|
|
@@ -29,6 +29,7 @@ import { | |
| getAllNotifications, | ||
| markAsRead, | ||
| markRepoAsRead, | ||
| markAllNotificationsAsRead, | ||
| } from '../index'; | ||
|
|
||
| const mapStateToProps = state => ({ | ||
|
|
@@ -40,6 +41,8 @@ const mapStateToProps = state => ({ | |
| isPendingUnread: state.notifications.isPendingUnread, | ||
| isPendingParticipating: state.notifications.isPendingParticipating, | ||
| isPendingAll: state.notifications.isPendingAll, | ||
| isPendingMarkAllNotificationsAsRead: | ||
| state.notifications.isPendingMarkAllNotificationsAsRead, | ||
| }); | ||
|
|
||
| const mapDispatchToProps = dispatch => | ||
|
|
@@ -50,17 +53,20 @@ const mapDispatchToProps = dispatch => | |
| getAllNotifications, | ||
| markAsRead, | ||
| markRepoAsRead, | ||
| markAllNotificationsAsRead, | ||
| }, | ||
| dispatch | ||
| ); | ||
|
|
||
| const styles = StyleSheet.create({ | ||
| buttonGroupWrapper: { | ||
| backgroundColor: colors.greyLight, | ||
| paddingTop: Platform.OS === 'ios' ? 28 : 15, | ||
| paddingTop: Platform.OS === 'ios' ? 30 : 10, | ||
| paddingBottom: 10, | ||
| }, | ||
| buttonGroupContainer: { | ||
| height: 30, | ||
| marginTop: 0, | ||
| }, | ||
| buttonGroupText: { | ||
| ...fonts.fontPrimaryBold, | ||
|
|
@@ -70,7 +76,7 @@ const styles = StyleSheet.create({ | |
| }, | ||
| repositoryContainer: { | ||
| padding: 0, | ||
| marginVertical: 25, | ||
| marginBottom: 15, | ||
| }, | ||
| headerContainer: { | ||
| flexDirection: 'row', | ||
|
|
@@ -110,6 +116,16 @@ const styles = StyleSheet.create({ | |
| textAlign: 'center', | ||
| ...fonts.fontPrimary, | ||
| }, | ||
| markAllAsReadButton: { | ||
| marginTop: 3, | ||
| paddingTop: 3, | ||
| paddingBottom: 3, | ||
| marginLeft: 11, | ||
| marginRight: 11, | ||
| borderColor: colors.mercury, | ||
| borderWidth: 1, | ||
| borderRadius: 3, | ||
| }, | ||
| }); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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/..) :)
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
|
|
||
| class Notifications extends Component { | ||
|
|
@@ -119,13 +135,15 @@ class Notifications extends Component { | |
| getAllNotifications: Function, | ||
| markAsRead: Function, | ||
| markRepoAsRead: Function, | ||
| markAllNotificationsAsRead: Function, | ||
| unread: Array, | ||
| participating: Array, | ||
| all: Array, | ||
| language: string, | ||
| isPendingUnread: boolean, | ||
| isPendingParticipating: boolean, | ||
| isPendingAll: boolean, | ||
| isPendingMarkAllNotificationsAsRead: boolean, | ||
| navigation: Object, | ||
| }; | ||
|
|
||
|
|
@@ -147,6 +165,16 @@ class Notifications extends Component { | |
| this.props.getAllNotifications(); | ||
| } | ||
|
|
||
| componentWillReceiveProps(nextProps) { | ||
| if ( | ||
| !nextProps.isPendingMarkAllNotificationsAsRead && | ||
| this.props.isPendingMarkAllNotificationsAsRead && | ||
| !this.isLoading() | ||
| ) { | ||
| this.getNotifications()(); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ()() ?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about rename
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is unlikely that the first name is fine (
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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"?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think something like
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @lex111 I prefer to keep Because there is another place calls this function, which just expects a function. After #360 is merged, I guess |
||
| } | ||
| } | ||
|
|
||
| getImage(repoName) { | ||
| const notificationForRepo = this.notifications().find( | ||
| notification => notification.repository.full_name === repoName | ||
|
|
@@ -316,7 +344,7 @@ class Notifications extends Component { | |
|
|
||
| render() { | ||
| const { type } = this.state; | ||
| const { language } = this.props; | ||
| const { language, markAllNotificationsAsRead } = this.props; | ||
|
|
||
| const repositories = [ | ||
| ...new Set( | ||
|
|
@@ -330,6 +358,8 @@ class Notifications extends Component { | |
| return a.toLowerCase() > b.toLowerCase() ? 1 : -1; | ||
| }); | ||
|
|
||
| const isEmptyNotifications = this.notifications().length === 0; | ||
|
|
||
| return ( | ||
| <ViewContainer> | ||
| <View style={styles.container}> | ||
|
|
@@ -346,25 +376,41 @@ class Notifications extends Component { | |
| selectedTextStyle={styles.buttonGroupTextSelected} | ||
| containerStyle={styles.buttonGroupContainer} | ||
| /> | ||
|
|
||
| <Button | ||
| icon={{ | ||
| name: 'check', | ||
| size: 20, | ||
| type: 'octicon', | ||
| color: colors.shark, | ||
| }} | ||
| title={translate('notifications.main.markAllAsRead')} | ||
| buttonStyle={styles.markAllAsReadButton} | ||
| color={colors.shark} | ||
| backgroundColor={colors.white} | ||
| textStyle={styles.buttonGroupText} | ||
| onPress={() => markAllNotificationsAsRead()} | ||
| disabled={isEmptyNotifications} | ||
| /> | ||
| </View> | ||
|
|
||
| {this.isLoading() && | ||
| this.notifications().length === 0 && | ||
| isEmptyNotifications && | ||
| <LoadingContainer | ||
| animating={this.isLoading() && this.notifications().length === 0} | ||
| animating={this.isLoading() && isEmptyNotifications} | ||
| text={translate('notifications.main.retrievingMessage', language)} | ||
| style={styles.marginSpacing} | ||
| />} | ||
|
|
||
| {!this.isLoading() && | ||
| this.notifications().length === 0 && | ||
| isEmptyNotifications && | ||
| <View style={styles.textContainer}> | ||
| <Text style={styles.noneTitle}> | ||
| {translate('notifications.main.noneMessage', language)} | ||
| </Text> | ||
| </View>} | ||
|
|
||
| {this.notifications().length > 0 && | ||
| {!isEmptyNotifications && | ||
| <FlatList | ||
| ref={ref => { | ||
| this.notificationsList = ref; | ||
|
|
||

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