-
Notifications
You must be signed in to change notification settings - Fork 3
Add lock and rename content functionality to Publisher Command Center #138
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
Conversation
| ]), | ||
| m("input", { | ||
| oninput: function(e) { | ||
| RenameModalForm.isValid = e.target.validity.valid; |
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.
| url: `api/content/${guid}/lock`, | ||
| }).then((response) => { | ||
| const targetContent = this.data.find((c) => c.guid === guid); | ||
| Object.assign(targetContent, response); |
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.
Since Mihtril.JS doesn't have reactivity like Vue or React, we need to change the content of the data in order to reflect the new changes to the UI via the automatic m.redraw(). Without this, we would need a page reload to refresh the UI.
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.
Good call adjusting the data so we don't need the slower API call.
| visitor = get_visitor_client(posit_connect_user_session_token) | ||
| content = visitor.content.get(content_id) |
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.
My first thought with both of these was "do we already have the content data?". If we do we could avoid an API call here.
It looks like there isn't a good way, given only a GUID, to call content.update(title = title) from the SDK though, perhaps that is something we could request to make things like this a bit easier so this could be faster.
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 there is!
We can do:
visitor = get_visitor_client(posit_connect_user_session_token)
context = connect.Context(visitor)
content = connect.ContentItem(context, content_id)
content.update(title = title)with the additional imports:
from posit.connect.context import Context
from posit.connect.content import ContentItem
We can make a Context for the ContentItem and pass the guid. Since ContentItem only needs a Context and guid and all other attributes are optional this is enough to do the rename.
The lock has is_locked = content.locked which relies on the Content's locked attribute. If we can pass that in the FastAPI /api/content/{content_id}/lock API (or create two one for unlock and one for lock) then we can do the same there speeding it up by 2 times.
I don't think any of this is necessary in this PR to be fair. If it is very easy we can get it in otherwise we can follow-up since it would be good deal faster.
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 looks good @dotNomad! For what it's worth, I wouldn't worry about the additional API call to retrieve the content in order to update it. It's all happening across the local network, so the overhead is fairly negligible.
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.
For what it's worth, I wouldn't worry about the additional API call to retrieve the content in order to update it. It's all happening across the local network, so the overhead is fairly negligible.
Oh that is a good point - it will be much faster than local dev 👍
| m(LockContentButton, { | ||
| contentId: guid, | ||
| contentTitle: title, | ||
| isLocked: content["locked"], |
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 sweet we have isLocked for the component, we can use that in Contents.lock and send it up to the API (or have two endpoints)
| m("a", { | ||
| class: "fa-solid fa-arrow-up-right-from-square", | ||
| href: content["content_url"], | ||
| title: `Open ${title} (opens in new tab)`, |
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.
| title: `Open ${title} (opens in new tab)`, | |
| title: `Open ${title}`, |
The "(opens in a new tab)" feels a bit verbose to me, but I'm not that adamant about removing it.
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 can move the "(opens in new tab)" portion to the aria-label instead of the tooltip. In general, if a link opens a new window or tab, the screen reader should alert the user of the action.
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.
In general, if a link opens a new window or tab, the screen reader should alert the user of the action.
Do screen readers not give that information when target="_blank"? I guess they don't
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.
Added aria-label in commit 3e93e62.
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 screen readers not give that information when target="_blank"? I guess they don't
Unfortunately, not. It would be a nice add-on to automatically provide that information.
| import m from "mithril"; | ||
| import Contents from "../models/Contents"; | ||
|
|
||
| const LockedContentButton = { |
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 found this button a bit tricky on first use and accidentally clicked it twice - partly because the API was slow since we do two Connect calls, but also because there wasn't any feedback.
I did a quick edit of the component that may be a good starting point for adjust this
Code
import m from "mithril";
import Contents from "../models/Contents";
const LockedContentButton = {
oninit: function () {
this.isLoading = false;
},
view: function (vnode) {
const labelMessage = vnode.attrs.isLocked ?
`Unlock ${vnode.attrs.contentTitle}` :
`Lock Content ${vnode.attrs.contentTitle}`;
const iconClassName = vnode.attrs.isLocked ? "fa-lock" : "fa-lock-open";
return m("button", {
class: "action-btn" + (this.isLoading ? " disabled" : ""),
ariaLabel: labelMessage,
title: labelMessage,
disabled: this.isLoading,
onclick: async () => {
if (this.isLoading) return;
this.isLoading = true;
m.redraw();
try {
await Contents.lock(vnode.attrs.contentId);
} finally {
this.isLoading = false;
m.redraw();
}
}
}, [
m("i", {
class: `fa-solid ${this.isLoading ? "fa-spinner fa-spin" : iconClassName}`
})
])
}
};
export default LockedContentButton;This changes the button to a disabled spinner while the API call is going. This made is much easier to tell what was going on and avoid a double lock call.
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.
Added improvements to the locking/unlocking functionality in commit 3e93e62.
| const modal = Modal.getInstance(modalEl); | ||
| modal.hide(); | ||
|
|
||
| RenameModalForm.newName = ""; |
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.
Good catch!
| url: `api/content/${guid}/lock`, | ||
| }).then((response) => { | ||
| const targetContent = this.data.find((c) => c.guid === guid); | ||
| Object.assign(targetContent, response); |
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.
Good call adjusting the data so we don't need the slower API call.
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.
Both #146 and this PR are seeking to upgrade Publisher Command Center. Lets get both merged, then do a version bump. So we can take the manifest.json changes out of this PR.
43074b8 to
8efa8c0
Compare
dotNomad
left a comment
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 fantastic thank you for those changes 🎉

Intent
Addresses the last two items of the #122
Approach
Created two endpoints to handle locking/unlocking content and rename content.
Example
Publisher Command Center Test on DogFood
Screenshots