-
Notifications
You must be signed in to change notification settings - Fork 15
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
Api key expiration frontend notifications #2388
Conversation
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.
Code apart, the toasts look good to me Carmine!
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.
LGTM @CDimonaco , only few minor things. Also, would it make sense to have the DismissibleToast component in storybook?
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 think the correct word would be Dismissible
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.
We can change it, I googled and the two forms are both valid, idk
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 prefer Dismissable
. It looks more correct in this case
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.
Really nice @CDimonaco
I left some nitpick comments.
I guess that the visualization of the api key itself in the settings page will have its own sagas/queries, right?
expect(dispatched).toEqual([]); | ||
}); | ||
|
||
it('should dispatch an expired key notification if the expire_at of the api key is in the past', async () => { |
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.
for me these test description texts are a bit awkard, but well, i can live with that XD
@rtorrero Storybook for the component it's not needed, it's not a real "visual" component, in storybook is just a div without style, it's meant to be used inside the toast library wrapper, who injects styles and icon |
524b9eb
to
8c2d9d8
Compare
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.
Thank you for the updates @CDimonaco
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 prefer Dismissable
. It looks more correct in this case
Is it possible to add this to Storybook? I would like review it in the browser. 😅 |
It's completely useless to have in storybook like I was saying in the other comment, it's an unstyled piece of html, the result is in the screenshot and the library itslef provide the style to the toast, I can put a button in storybook to spawn a toast but Idk if it's really useful |
Sorry I missed that comment. I would find it useful to review the component and just make sure it renders the same and that there will be consistency in the future should changes occur. |
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.
Hey, well done, besides the small comments left by others nothing to add.
LGTM!
c0d1b0d
to
ea73402
Compare
Description
When the trento frontend application loads, we fetch the api key expiration.
If the api key is expired, the frontend shows a persistent toast.
If the api key will expire in 30 days, the frontend shows a dismissable toast.
This pr also adds a dismissable toast, and a custom notification dispatching with custom duration and dismissable.
How was this tested?
Automated tests, e2e tests