-
Notifications
You must be signed in to change notification settings - Fork 297
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
[DDW-509] Show success message for saving logs #1228
[DDW-509] Show success message for saving logs #1228
Conversation
@daniloprates good idea! like it :) |
actions.profile.downloadLogs.trigger({ fileName, destination, fresh: true }); | ||
} | ||
const { id, duration } = this.notification; | ||
actions.notifications.open.trigger({ id, duration, }); |
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.
You forgot to check if the downloaded logs are actually successfully saved before showing the notification!
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.
You are right! Do you think listening to the action is enough or we should actually check if the file exists? I think listening is enough.
} | ||
const { id, duration } = this.notification; | ||
actions.notifications.open.trigger({ id, duration, }); |
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.
You forgot to check if the downloaded logs are actually successfully saved before showing the notification!
Was wondering if a "View in Finder/Open folder" button wouldn't be useful, @a-rukin @nikolaglumac? |
This may cause more bad than good. |
@a-rukin fair enough :) |
@daniloprates do we have translations for this PR? |
|
@daniloprates @darko-mijic I suggest we use the following text for the success message: |
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 and works great @daniloprates 🎉
This PR implements a success message for logs download.
Todo:
Screenshots:
Review Checklist:
Basics
yarn run test
)yarn run dev
)yarn run package
/ CI builds)yarn run flow:test
)yarn run lint
)yarn run manage:translations
produces no changes)yarn run storybook
)yarn.lock
file is updatedCode Quality
Testing
After Review:
done
on the Youtrack board