Skip to content
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

feat: toggle run all notifications #1162

Merged
merged 3 commits into from
Feb 14, 2023

Conversation

lilyli9
Copy link
Contributor

@lilyli9 lilyli9 commented Feb 14, 2023

Added a toggle to the run all confirmation page so that users can choose whether they want a notification to be sent upon run completion. By default, the notification and toggle will be enabled to match previous behavior of Querybook where a notification will always be sent to the user upon run completion. The type of notification sent is still based on "notification preference" in user settings.

Screen Shot 2023-02-13 at 4 00 06 PM

@@ -381,26 +381,32 @@ def run_data_doc(id):


@register("/datadoc/<int:id>/run/", methods=["POST"])
def adhoc_run_data_doc(id):
def adhoc_run_data_doc(id, sendNotification=True):
Copy link
Collaborator

Choose a reason for hiding this comment

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

use camel casing send_notifications
also please default it as False


interface IProps {
defaultNotification: boolean;
handleNotificationChange: (notification: boolean) => void;
Copy link
Collaborator

Choose a reason for hiding this comment

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

onNotificationChange

const [notification, setNotification] = useState(defaultNotification);

const internalNotificationChange = useCallback(
(value) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: (value: boolean) =>

onChange={internalNotificationChange}
/>

<span data-balloon-pos={'up'} className="ml4">
Copy link
Collaborator

Choose a reason for hiding this comment

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

data-balloon-pos={'up'}
but there is no tooltip to show?

@lilyli9 lilyli9 requested a review from czgu February 14, 2023 16:55
Copy link
Collaborator

@czgu czgu left a comment

Choose a reason for hiding this comment

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

amazing!

@czgu czgu merged commit 53a70e1 into pinterest:master Feb 14, 2023
rohan-sh1 pushed a commit to CAI-TECHNOLOGIES/cai-ext-db-explorer that referenced this pull request Apr 11, 2023
* feat: toggle run all notifications

* chore: rename var change default send notification

* fix: rename all handleNotificationChange
aidenprice pushed a commit to arrowtail-precision/querybook that referenced this pull request Jan 3, 2024
* feat: toggle run all notifications

* chore: rename var change default send notification

* fix: rename all handleNotificationChange
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants