Eng/notion messaging post update permissions #4221
Conversation
fontanierh
force-pushed
the
eng/notion-massaging-post-update-permissions
branch
from
ebcbd77 to
6435a64
Compare
fontanierh
changed the title
| <DataSourceDetailsModal | ||
| dataSource={dataSource} | ||
| visible={showDataSourceDetailsModal} | ||
| onClose={() => { | ||
| setShowDataSourceDetailsModal(false); | ||
| }} | ||
| onClick={() => { | ||
| void handleUpdatePermissions(); | ||
| void handleUpdatePermissions().then(() => { |
There was a problem hiding this comment.
nit: I think we had something saying from now on we need to catch?
There was a problem hiding this comment.
In client-side code too ? I'm not sure what I would be doing with it (console.error is not actionable for the user)
| @@ -725,6 +727,7 @@ interface ConnectorUiConfig { | |||
| addDataButtonLabel: string | null; | |||
| displaySettingsButton: boolean; | |||
| guideLink: string | null; | |||
| onPostPermissionsUpdate?: () => ReactNode; | |||
There was a problem hiding this comment.
here, you're passing content
but convention is usually onSomething = handler on event something, somethingCallback => happens after something is done
I would do postPermissionsUpdateMessage?: ReactNode here
More generic but needs a bit refactoring thinking would be permissionsUpdateCallback?: () => void (but not sure it can work
There was a problem hiding this comment.
Agreed, changing.
There was a problem hiding this comment.
Changed to just return a string as well.
Description
fixes https://github.com/dust-tt/tasks/issues/161
Risk
Deploy Plan