-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
#1801 Added close a specific notification feature #1985
#1801 Added close a specific notification feature #1985
Conversation
@jrobinAV can you review my pr please? |
I have put @FabienLelaquais and @FredLL-Avaiga as reviewers of this PR. |
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.
Waiting eagerly for the frontend part of it
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
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.
waiting for the frontend :-D
@quest-bot loot #1801 |
Quest PR submitted!@AdeshGhadage, you are attempting to solve the issue and loot this Quest. Will you be successful? Questions? Check out the docs. |
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.
Looks very good, tx @AdeshGhadage
- a few changes
- some tests
please update your branch |
any question ? |
is my pr correct? any suggestion or changed i have missed ? |
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.
A few comments
- Would be nice to have an example that shows how to get the returned notification_id and delete the notification (/doc/gui/examples)
thanks to makes the resolved discussions as resolved and to update the branch |
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.
looks almost good
Just needs a few tests
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.
Very good
Now we need some tests:
- backend to check that the ws payload looks OK
- frontend around the reducer and Alerts (fix the ones that are broken + ?)
Branch updated |
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.
an old test is now failing, can you fix it ?
const action = { type: Types.DeleteAlert };
644 | const newState = taipyReducer(initialState, action);
> 645 | expect(newState.alerts).toEqual([
| ^
646 | {
647 | message: "alert2",
648 | atype: "type2",
PS: I've updated your branch |
why its giving empty array as during test? can help resolve this issue? |
Because there's no id passed in the deleteAction so every element of the array is filtered out |
there is some issue with nanoid call i tried to mock the nanoid library by using
but still not worked so i put random string |
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.
Excellent work
Tx @AdeshGhadage
Can you merge this @jrobinAV ? |
This pull request addresses issue #1801
Hello Maintainers,
Could you please review my code? I would appreciate any suggestions or feedback you might have.