Skip to content

Conversation

@PrakashDurlabhji
Copy link
Contributor

No description provided.

Copy link
Collaborator

@maxceem maxceem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@PrakashDurlabhji

Provided fix doesn't work for me. As soon as we open the dropdown with notification, all the notifications should be marked as seen and red cue should disappear, but it doesn't disappear for me, see video https://monosnap.com/file/618NC2xK5cxoZwi0rqSZSwRFQkqFxL

I see there is only one small change in the PR, did you submit all the update code?

@PrakashDurlabhji
Copy link
Contributor Author

@maxceem
1.can you please confirm if "isOpen" was ever working in NotificationDropDownContainer?
because suddenly there is an issue.
2. also, if route "notification/seen" exists?

@maxceem
Copy link
Collaborator

maxceem commented Jul 8, 2019

@PrakashDurlabhji

1.can you please confirm if "isOpen" was ever working in NotificationDropDownContainer?
because suddenly there is an issue.

You may check the current version on https://connect.topcoder-dev.com/ - I don't see any issues with isOpen.

  1. also, if route "notification/seen" exists?

Do you mean the API URL to mark notifications seen? The url is https://api.topcoder-dev.com/v5/notifications/${id}/seen, where ${id} - is id of the notification. We already have a service method for it https://github.com/appirio-tech/connect-app/blob/dev/src/routes/notifications/services/notifications.js#L20

@PrakashDurlabhji
Copy link
Contributor Author

@maxceem update done

Copy link
Collaborator

@maxceem maxceem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for update @PrakashDurlabhji

I cannot run code with the latest update, getting an error:

image

Does it work for you?

@PrakashDurlabhji
Copy link
Contributor Author

@maxceem i making a commit, well tested and red cue disappears perfectly on opening notifications by clicking on bell.
just 10 mins more

@maxceem
Copy link
Collaborator

maxceem commented Jul 10, 2019

@PrakashDurlabhji good, I'll review as soon as you post an update.

@PrakashDurlabhji
Copy link
Contributor Author

@maxceem commit updated. kindly verify thank you

Copy link
Collaborator

@maxceem maxceem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @PrakashDurlabhji after the last update works great.

There are some code issues to fix before I can merge it. Please, have a look at the 3 code comments.

Also, there are a couple of other code issues:

  1. There are lint errors:

    image

  2. As we don't use lastVisited visited anymore we should remove all the related code which is not more in use. Like lastVisited from the redux store, visitNotifications action, onVisitNotifications method, VISIT_NOTIFICATIONS action and so on

@PrakashDurlabhji
Copy link
Contributor Author

@maxceem all updated

Copy link
Collaborator

@maxceem maxceem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost perfect @PrakashDurlabhji

There are is a couple of places with lastVisited left (which is not in use anymore):

image

Also, VISIT_NOTIFICATIONS is left

image

and it causes a warning:

image

@maxceem
Copy link
Collaborator

maxceem commented Jul 12, 2019

@PrakashDurlabhji one lint error is left, also VISIT_NOTIFICATIONS is not removed completely.

image

Copy link
Collaborator

@maxceem maxceem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything is good now @PrakashDurlabhji.

Thanks.

@maxceem maxceem merged commit fd57215 into topcoder-archive:cf18 Jul 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants