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.

Sign up for GitHub

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

[BREAK] Change apps/icon endpoint to return app's icon and use it to show on Ui Kit modal #16522

Merged
merged 4 commits into from
Feb 8, 2020
+22 −17
Merged

[BREAK] Change apps/icon endpoint to return app's icon and use it to show on Ui Kit modal #16522

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
982ee13
Use new endpoint to get app icon
sampaiodiego Feb 7, 2020
fb5d0d9
Use old URL
sampaiodiego Feb 8, 2020
be3b6c4
Merge branch 'develop' into improve-app-icon
sampaiodiego Feb 8, 2020
d66b923
Use getURL
sampaiodiego Feb 8, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 0 additions & 6 deletions app/apps/server/bridges/uiInteraction.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,6 @@ export class UiInteractionBridge {
throw new Error('Invalid app provided');
}

const { name, iconFileContent } = app.getInfo();

Object.assign(interaction, {
appInfo: { name, base64Icon: iconFileContent },
Copy link
Member

Choose a reason for hiding this comment

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

༼ つ ◕_◕ ༽つ name

});

Notifications.notifyUser(user.id, 'uiInteraction', interaction);
}
}
27 changes: 27 additions & 0 deletions app/apps/server/communication/rest.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -516,6 +516,33 @@ export class AppsRestApi {
},
});

this.api.addRoute('icon/:id', { authRequired: false }, {
Copy link
Member Author

Choose a reason for hiding this comment

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

I know I can be hated for this because there is another endpoint :id/icon already: https://github.com/RocketChat/Rocket.Chat/pull/16522/files#diff-1d7b87614deb7cd95b3cfaa7420842a7R506

Copy link
Member

Choose a reason for hiding this comment

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

IIRC the other one is not used anywhere? 🤔 I think I've searched for it before and haven't found anything. If that's the case, you can just change the behavior of the other one 😬

Copy link
Member Author

Choose a reason for hiding this comment

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

Since we're targeting a major version, I could introduce this breaking change.

get() {
const prl = manager.getOneById(this.urlParams.id);
if (!prl) {
return API.v1.notFound(`No App found by the id of: ${ this.urlParams.id }`);
}

const info = prl.getInfo();
if (!info || !info.iconFileContent) {
return API.v1.notFound(`No App found by the id of: ${ this.urlParams.id }`);
}

const imageData = info.iconFileContent.split(';base64,');

const buf = Buffer.from(imageData[1], 'base64');

return {
statusCode: 200,
headers: {
'Content-Length': buf.length,
'Content-Type': imageData[0].replace('data:', ''),
},
body: buf,
};
},
});

this.api.addRoute(':id/languages', { authRequired: false }, {
get() {
const prl = manager.getOneById(this.urlParams.id);
Expand Down
3 changes: 1 addition & 2 deletions app/ui-message/client/blocks/MessageBlock.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,6 @@ const textParser = uiKitText(new class {
return text;
}
}());
const thumb = 'data:image/gif;base64,R0lGODlhAQABAIAAAMLCwgAAACH5BAAAAAAALAAAAAABAAEAAAICRAEAOw==';

// https://www.w3.org/TR/wai-aria-practices/examples/dialog-modal/dialog.html

Expand Down Expand Up @@ -134,7 +133,7 @@ export const modalBlockWithContext = ({
<Modal open id={id} ref={ref}>
<Modal.Header>
{/* <Modal.Thumb url={`api/apps/${ context.appId }/icon`} /> */}
<Modal.Thumb url={thumb} />
<Modal.Thumb url={`/api/apps/icon/${ data.appId }`} />
Copy link
Member Author

Choose a reason for hiding this comment

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

this is not the best way to get the full URL, pls advice

Copy link
Member

Choose a reason for hiding this comment

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

there is a context to set the prefix path

Copy link
Member Author

Choose a reason for hiding this comment

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

I have found:

Rocket.Chat/client/contexts/ServerContext.js

Line 12 in 982ee13

export const useAbsoluteUrl = () => useContext(ServerContext).absoluteUrl;

but looks like I cannot use it because you're not using ServerProvider that implements it:

Rocket.Chat/client/providers/ServerProvider.js

Line 42 in 982ee13

export function ServerProvider({ children }) {

what should I do?

Copy link
Member Author

Choose a reason for hiding this comment

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

idk the advantages using a context, so I changed to use getURL that is widely used.

<Modal.Title>{textParser([title])}</Modal.Title>
<Modal.Close tabIndex={-1} onClick={onClose} />
</Modal.Header>
Expand Down