-
Notifications
You must be signed in to change notification settings - Fork 7
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
Copy & Paste of Activity Directives between Plans #1627
base: develop
Are you sure you want to change the base?
Conversation
fab1cfa
to
e5c2ec1
Compare
function canPasteActivityDirectives(): Promise<number> { | ||
return new Promise(resolve => { | ||
getActivityDirectivesClipboardCount().then(result => { | ||
if (plan !== null && hasCreatePermission && result > 0) { |
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 it's okay to use hasCreatePermission
here, but our convention for permissions has been to disallow the user from being able to click on the option to paste and displayed a tooltip stating why they're unable to click. It just lets them know quicker why something isn't available to them.
<ContextMenuItem on:click={pasteActivityDirectives}>{getPasteActivityDirectivesText()}</ContextMenuItem> | ||
<ContextMenuSeparator></ContextMenuSeparator> | ||
{/if} | ||
{#await canPasteActivityDirectives() then howMany} |
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.
Can we keep this option always visible, but disabled if they don't have anything to paste in?
src/utilities/clipboard.ts
Outdated
try { | ||
window.navigator.clipboard.readText().then(clipText => resolve(clipText)); | ||
} catch (e) { | ||
resolve(`{}`); //empty object string? |
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.
Does this need to be an object string or can we just return null
?
e5c2ec1
to
258e0ba
Compare
258e0ba
to
3796dac
Compare
@duranb addressed feedback! and actually simplified it! Give it a look when you can. |
src/utilities/activities.ts
Outdated
} | ||
} catch (e) { | ||
console.error(e); | ||
return false; | ||
//eh silent |
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.
Any reason not to spit out a log here?
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
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.
Actually I remember why we keep this silent. This throws an error if the clipboard has something that is not json, like general string/clipboard things. It should fail silently.
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.
Oh makes sense. Could you update the comment there with a note about that so that future us/others don't throw a log there?
return 'You do not have permission create activity directives'; | ||
} else if (directivesInClipboard <= 0) { | ||
return 'No activity directives in clipboard'; | ||
} |
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.
Can you add a final return statement for if the logic falls through all the conditionals?
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 you mind saving the permissionError
to a variable that gets updated in a reactive statement? We generally save the error to a local state variable so that it's calculated only when the dependent variables change and not every time the component re-renders.
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.
The showCopyMenu
variable seems to no longer be being set to anything else in this file. Can we remove it and just pass true
into the BulkActionDataGrid
's showCopyMenu
prop?
@@ -432,18 +428,35 @@ | |||
> | |||
Set Simulation End | |||
</ContextMenuItem> | |||
{#if canPasteActivityDirectives()} | |||
{#await getActivityDirectivesClipboardCount() then directivesInClipboard} |
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.
Is there still a chance that this menu item won't show while it's loading the count? Is it always quick to pop in?
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'm sure there's still a chance if you're trying to copy/paste tens of thousands of activities
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.
Yeah that makes sense. Can we instead save the count to a variable and instead of await
ing to show the menu option, just show the option with a 0
count and disabling it to be consistent with always showing options?
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.
@duranb since we're using the system clipboard we first need to make sure the clipboard has activity directives and then we get how many there are. So keeping the count doesn't really help me because we still have to deserialize the clipboard to see if it's the right type of content.
src/utilities/clipboard.ts
Outdated
@@ -0,0 +1,26 @@ | |||
export function setClipboardContent(content: object, success?: () => void, fail?: () => void) { | |||
navigator.permissions.query({ name: 'clipboard-write' as PermissionName }).then(result => { |
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.
PermissionName
is missing a type definition
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.
@duranb not sure I understand this one!
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'm getting PermissionName
as an unresolved type from typescript.
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.
Ok it looks like we dont need this after all so i removed that section.
src/utilities/clipboard.ts
Outdated
}); | ||
} | ||
|
||
export async function getClipboardContent(): Promise<string | undefined> { |
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 this needs to return void
instead of undefined
.
3796dac
to
0f7ae85
Compare
0f7ae85
to
848dfbe
Compare
|
Extending the original Copy & Paste feature with a few things here...