-
Notifications
You must be signed in to change notification settings - Fork 11.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
PublicDashboards: Email sharing #63762
Conversation
…banas/emailSharingFeature
…m:grafana/grafana into juanicabanas/sharePublicDashboardRefactor
…banas/emailSharingFeature
// @ts-ignore | ||
return requestOptions.manageError ? requestOptions.manageError(error) : { error }; |
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.
Do not disable the type checker. This will break Grafana in unknown ways. We cannot introduce more type holes and unreliability.
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.
Instead, I would suggest to revert manageError
to accept unknown again, and within your getConfigError
/manageError
function, you can check if it has a status
field after using isFetchError
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 didn't know about that isFetchError function. Looks better now!
Making some little modifications |
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 made a number of style/formatting edits.
@owensmallwood for this v1 we don't support a bulk operation |
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.
Changes look good!
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 great 👍 Nice work!
Feature for sharing a public dashboard by email
What is this feature?
Email sharing option is added for enterprise.
Once enabled, you will be able to share a public dashboard just for the emails you added in the configuration modal.
Why do we need this feature?
It adds a new way of sharing a public dashboard.
Who is this feature for?
Enterprise users
Which issue(s) does this PR fix?:
Fixes #4402, #4401, #4399
email-sharing-demo.mov