Skip to content

Notifications#27

Open
kristofbc wants to merge 4 commits into
developfrom
ft-notification-integration
Open

Notifications#27
kristofbc wants to merge 4 commits into
developfrom
ft-notification-integration

Conversation

@kristofbc
Copy link
Copy Markdown
Collaborator

Question Answer
Bugs correction ? no
New features ? yes
Related issues close #26

What have you changed ?

The project page with the new notification button

How did you do that ?

A new NotificationService was added to communicate with the new /notification endpoint of the server;
A new page was also added and is used to display read and unread notification that you are the recipient.

Important, it cannot work without the back-end: ColabTask/api-tasklist#17

@kristofbc kristofbc added this to the End of school project milestone Mar 23, 2017
@kristofbc kristofbc requested a review from RignonNoel March 23, 2017 04:11
@ghost ghost assigned kristofbc Mar 23, 2017
@ghost ghost added the In review label Mar 23, 2017
Copy link
Copy Markdown
Collaborator

@RignonNoel RignonNoel left a comment

Choose a reason for hiding this comment

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

After full review it seems you want to include the possibility to put a notification as Unread. I really not against this choice but i don't understant the value, for me it's seems just strange.

Otherwise all the rest seems good to me, just check this little artefact to be sure your good with that.

Good work 👍

constructor(public storage: Storage, protected http:Http, private _config:Config) {
this.serverURL = _config.get('apiUrl');
storage.get('user_id').then(
(user_id) => {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think it's an artefact of your first urls architecture on the API because you don't need the user_id in this line.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Indeed, my bad!

@kristofbc
Copy link
Copy Markdown
Collaborator Author

@RignonNoel I removed the unused check!
Also, I added the read/unread feature thinking that, maybe you've 'read' a notification but then, you want to save it for later, so you 'unread' it. Like a email!

Copy link
Copy Markdown
Collaborator

@RignonNoel RignonNoel left a comment

Choose a reason for hiding this comment

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

Yeah, it's not a bad idea. ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add a notification when a task is created and assigned to someone else

2 participants