feat(tabbar): show notifications count #360

lex111 · 2017-09-25T20:55:10Z

So now the lazy loading of tabs works, then probably it makes sense to display the number of notifications in the tabbar.

This is the early version, you still need to deduct the notifications that were read by the repository (you need to determine how many they need to be deducted, and do it by analogy here).

Quick demo:

If there are more than 99 notifications, 99+ will be displayed.

Also I had an idea with auto-updates of the number of notifications? Is it necessary?

We can also think about how to make this component differently, more precisely how to connect redux and react-navigation? I had an idea to make a component of the TabBarIcon type with the badgeText property and pass the count there, but it's not clear how to connect redux then to update the badge text.

Feel free to suggest changes, if you want.

lex111 · 2017-09-26T09:15:02Z

I made update the badge after mark the repo notifications, and also changed a bit of the icon design:

> 99	< 99

How do you like the idea?

machour · 2017-09-26T09:51:53Z

src/config/colors.js

@@ -17,6 +17,7 @@ export const colors = {
  darkGreen: '#27ae60',
  red: '#ee0701',
  darkRed: '#e74c3c',
+  redDarkest: '#da0000',


machour

Really nice addition!

machour · 2017-09-26T10:52:32Z

src/api/index.js

+    count = linkHeader.split('=').pop();
+  }
+
+  return +count;


what do +count do ? Oo

This convert the value to a number, similarly does the call to parseInt()

Nice, thanks

machour · 2017-09-26T10:53:04Z

src/api/index.js

+  const response = await fetch(ENDPOINT, accessTokenParameters(accessToken));
+
+  let linkHeader = response.headers.get('Link');
+  let count = 1;


if the Link header is not present, count should be the count of json objects returned in the response, not 1

See how this is handled in v3.count() I just added: 6ce8489#diff-284817f1b6aad47c0ac6d61bd72f474cR58

if the Link header is not present, count should be the count of json objects returned in the response, not 1

The fact is that when the notification in the repository is only one, the Link header will not be, and response.json().length will return 0, which is not correct. Therefore, the default is 1.

See how this is handled in v3.count() I just added: 6ce8489#diff-284817f1b6aad47c0ac6d61bd72f474cR58

Can you add a third parameter, such as defaultCount?

@lex111 as I understand, here's how the API respond :

if you have 0 notifications, response.status is 404 with a json body that must be ignored

if you have notifications :

if only 1, no Link header and response.json() is an array of 1 element (so json().length = 1)

if more, you parse the Link header to get the number

am I missing something?

Yes, that's right, I checked again, and found the error that you have now, and because of which I incorrectly put 1

response.json() is a promise, so it should be like this:

number = response.json().then(data => { return data.length; });

OMG, thanks! didn't know response.json() was a promise, I thought only response was.
I'll update count() to handle the 404, fix json().
Do you still think defaultValue is needed? I fail to see how

Do you still think defaultValue is needed? I fail to see how

No, at least for now.

just fixed the count() method and realized that json().length isn't needed at all as we request one element per page -_-

So, if 404 => 0 elements
If no Link header => 1 element
Else, count fetched from Link

I wonder if the 404 status code is sent for all empty request responses ..

(let's chat over gitter if you want)

lex111 · 2017-09-26T17:09:01Z

I made changes according with the feedback, and now I thought, can show the total number notifications, instead 99+? I did it because I wanted the badge to be always round, and if there is a four-digit number, then it will be oval.

machour · 2017-09-26T17:11:18Z

I personally think +99 is perfect

lex111 · 2017-09-26T17:13:27Z

Here is an example of how to make a badge:

machour · 2017-09-27T18:20:27Z

src/components/notification-icon.component.js

+        />
+
+        {notificationsCount
+          ? <View style={styles.badge}>


Hey @lex111, maybe we could use from react-native-elements which is already bundled :
https://react-native-training.github.io/react-native-elements/API/badge/

It's not round, I like that currenly it's just round in shape, well okay

Fair enough, if you would rather have a circular badge and think it looks neater I don't mind :)

I think we can do one of two things however:

Could we have a local badge component that contains the style for the badge and expose a background color and text color prop? Feels nice to have that exposed in it's own component. Would prefer if you call it circular-badge or something along those lines to not name-conflict with the react native elements badge component

OR we could use React Native Elements badge here and just apply containerStyle to make it circular.

I'm okay with either option 🙌

Strangely, I can not import this component:

import { Icon, Badge } from 'react-native-elements'; console.log(Badge); // undefined

I do not understand what's up, okay, I'll make a local component then.

andrewda · 2017-09-27T18:39:02Z

src/auth/screens/events.screen.js

@@ -86,8 +89,11 @@ class Events extends Component {
    }
  }

-  getUserEvents = (user = this.props.user) => {
+  getUserEvents = () => {


We're passing a user into this function on lines 82 and 88, and this would be therefore ignoring it. Maybe something like this will work?

getUserEvents = ({ user, accessToken } = this.props) => { // ... }

While changing lines 82 and 88 to pass in this.props and nextProps respectively.

I tried to do it, it did not work. In componentWillReceiveProps this will work.

Agree with @andrewda. Line 82 shouldn't matter since it's already defaulting to this.props.user (would prefer if you can remove the argument call in there (line 82) if you can too :)

But if you're removing the arguments entirely from the function declaration in line 92, line 88 doesn't look it would work at all. We're passing in nextProps.user but in here it's not being used at all..

Curious why

getUserEvents = ({ user, accessToken } = this.props) => { // ... }

doesn't work? Destructuring and applying a default should work if I'm not mistaken

housseindjirdeh

Beautiful stuff @lex111. Can't tell you how much I appreciate you always finding things to be improved in the app. Thank you.

Left a few comments but nothing major 👍

housseindjirdeh · 2017-09-28T02:14:02Z

src/auth/screens/events.screen.js

@@ -86,8 +89,11 @@ class Events extends Component {
    }
  }

-  getUserEvents = (user = this.props.user) => {
+  getUserEvents = () => {


Agree with @andrewda. Line 82 shouldn't matter since it's already defaulting to this.props.user (would prefer if you can remove the argument call in there (line 82) if you can too :)

But if you're removing the arguments entirely from the function declaration in line 92, line 88 doesn't look it would work at all. We're passing in nextProps.user but in here it's not being used at all..

Curious why

getUserEvents = ({ user, accessToken } = this.props) => { // ... }

doesn't work? Destructuring and applying a default should work if I'm not mistaken

housseindjirdeh · 2017-09-28T02:20:09Z

src/components/notification-icon.component.js

+        />
+
+        {notificationsCount
+          ? <View style={styles.badge}>


Fair enough, if you would rather have a circular badge and think it looks neater I don't mind :)

I think we can do one of two things however:

Could we have a local badge component that contains the style for the badge and expose a background color and text color prop? Feels nice to have that exposed in it's own component. Would prefer if you call it circular-badge or something along those lines to not name-conflict with the react native elements badge component

OR we could use React Native Elements badge here and just apply containerStyle to make it circular.

I'm okay with either option 🙌

housseindjirdeh · 2017-09-28T02:25:25Z

src/notifications/screens/notifications.screen.js

@@ -163,6 +167,8 @@ class Notifications extends Component {
    } = this.props;
    const { type } = this.state;

+    this.props.getNotificationsCount();


Could we have this as part of our destructured object up above:

const { getUnreadNotifications, getParticipatingNotifications, getAllNotifications, getNotificationsCount } = this.props;

So we can just call getNotificationsCount() here

housseindjirdeh · 2017-09-28T02:26:35Z

src/notifications/screens/notifications.screen.js

@@ -145,6 +148,7 @@ class Notifications extends Component {
    this.props.getUnreadNotifications();


Would prefer is we destructure here as well (I know this is slightly out of your scope however so don't worry if you would rather not):

const { getUnreadNotifications, getParticipatingNotifications, getAllNotifications, getNotificationsCount } = this.props;

And then Promise.all all the functions instead of calling them one by one

Done except promises, how can they help?

No worries, ideally Promise.all is used to make sure a collection of promises are complete before submitting an event after them but since we're not really doing anything after each of these specifically - I think it's okay if we don't include it here :)

lex111 · 2017-09-28T15:59:10Z

Ready for review.

housseindjirdeh

Works absolutely lovely mate 👏 Thank you!!!!!

housseindjirdeh

Just noticed one tiny thing to change, otherwise this baby works wonderfully :)

housseindjirdeh · 2017-09-28T18:19:34Z

src/auth/screens/events.screen.js

-  componentDidMount() {
-    if (this.props.user.login) {
-      this.getUserEvents(this.props.user);
+  componentDidMount({ user } = this.props) {


I don't think we should be including props as an argument to componentDidMount. We never call this method as a function anywhere in the component so there's no need to set an argument and define it's default (seems redundant).

Ideally, we can just destructure within the method itself:

componentDidMount() { const { user } = this.props; // .... }

Oops, that's right, I fixed it.

housseindjirdeh

Cheers, thanks so much for being on the ball and responding to change requests so quickly 🙌

feat(tabbar): show notifications count

fdb81ea

lex111 added ready for review 🏗️ work in progress labels Sep 25, 2017

lex111 requested review from machour, patw0929, housseindjirdeh and andrewda September 25, 2017 20:55

lex111 added 3 commits September 26, 2017 09:57

Merge remote-tracking branch 'origin' into notification-tabbar-icon

0bf9058

feat(notification-icon): update badge text after mark repo notifications

d30b9a8

feat(notification-icon): restyle

e34af6f

machour reviewed Sep 26, 2017

View reviewed changes

machour approved these changes Sep 26, 2017

View reviewed changes

machour requested changes Sep 26, 2017

View reviewed changes

machour mentioned this pull request Sep 26, 2017

Markdown rendering issues #363

Closed

lex111 added 3 commits September 26, 2017 19:49

Merge remote-tracking branch 'origin' into notification-tabbar-icon

b34db06

refactor(api): update according with new api

472a762

refactor(config): rename color

8f61ee1

lex111 removed the 🏗️ work in progress label Sep 27, 2017

machour reviewed Sep 27, 2017

View reviewed changes

andrewda requested changes Sep 27, 2017

View reviewed changes

housseindjirdeh requested changes Sep 28, 2017

View reviewed changes

lex111 added 5 commits September 28, 2017 16:56

refactor(notification-icon): extract Badge component

eff3e5a

refactor(events): use destructuring

54a3e67

refactor(notifications): use destructuring

29ef864

refactor(events): use destructuring

f9afbc8

fix(notification-icon): fix boolean check

caad62e

lex111 added 4 commits September 28, 2017 18:17

fix(fetchNotificationsCount): set zero as default count

4f4070b

fix(api count): return correct value if there is no linkHeader

00bc2b6

Merge remote-tracking branch 'origin' into notification-tabbar-icon

3552f91

fix(events): remove destructuring

f522c6b

lex111 mentioned this pull request Sep 28, 2017

feat(notifications): add button to mark all notifications as read #371

Merged

housseindjirdeh approved these changes Sep 28, 2017

View reviewed changes

housseindjirdeh requested changes Sep 28, 2017

View reviewed changes

refactor(events): remove destructuring

fa12608

housseindjirdeh approved these changes Sep 28, 2017

View reviewed changes

housseindjirdeh merged commit ec4610b into master Sep 28, 2017

lex111 deleted the notification-tabbar-icon branch September 28, 2017 20:43

		@@ -145,6 +148,7 @@ class Notifications extends Component {
		this.props.getUnreadNotifications();

feat(tabbar): show notifications count #360

feat(tabbar): show notifications count #360

Conversation

lex111 commented Sep 25, 2017 • edited

lex111 commented Sep 26, 2017 • edited

Choose a reason for hiding this comment

machour left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lex111 commented Sep 26, 2017

machour commented Sep 26, 2017

lex111 commented Sep 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lex111 Sep 27, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

housseindjirdeh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lex111 commented Sep 28, 2017

housseindjirdeh left a comment

Choose a reason for hiding this comment

housseindjirdeh left a comment

Choose a reason for hiding this comment

housseindjirdeh Sep 28, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

housseindjirdeh left a comment

Choose a reason for hiding this comment

lex111 commented Sep 25, 2017 •

edited

lex111 commented Sep 26, 2017 •

edited

lex111 Sep 27, 2017 •

edited

housseindjirdeh Sep 28, 2017 •

edited