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

[UI] Add support to download, publish, unpublish, and clone wasm support #7969

Merged
merged 5 commits into from
Jun 30, 2023

Conversation

Chadha93
Copy link
Member

@Chadha93 Chadha93 commented Jun 28, 2023

Signed-off-by: Gaurav Chadha gauravchadha1676@gmail.com
Notes for Reviewers
Add support to download, publish, unpublish, and clone wasm support.

WASM.mp4

This PR fixes #CU-863h1z6br

Signed commits

  • Yes, I signed my commits.

Signed-off-by: Gaurav Chadha <gauravchadha1676@gmail.com>
Signed-off-by: Gaurav Chadha <gauravchadha1676@gmail.com>
Signed-off-by: Gaurav Chadha <gauravchadha1676@gmail.com>
@l5io
Copy link
Collaborator

l5io commented Jun 28, 2023

Task linked: CU-863h1z6br WASM Filters

@Chadha93 Chadha93 requested a review from leecalcote June 28, 2023 13:02
@github-actions github-actions bot added the component/ui User Interface label Jun 28, 2023
@github-actions
Copy link

github-actions bot commented Jun 28, 2023

Signed-off-by: Gaurav Chadha <gauravchadha1676@gmail.com>
@Chadha93
Copy link
Member Author

@Abhishek-kumar09 @theBeginner86, all the filters are having the visibility as private...

@theBeginner86
Copy link
Member

theBeginner86 commented Jun 28, 2023

@Abhishek-kumar09 @theBeginner86, all the filters are having the visibility as private...

Yes that's true. It's because we haven't published any Filters to catalog so far.

Here, publishing to catalog means publishing from Meshery UI using our dispatch workflow and setting visibility as published. The ones which we see at https://meshery.io/catalog/webassembly are the hardcoded ones.

@leecalcote
Copy link
Member

@aabidsofi19 what comments do you have here?

@@ -12,6 +12,10 @@ export default function downloadFile({ id, type,name, source_type }) {
dataUri = `/api/pattern/download/${id}`;
}

if (type === "filter") {
Copy link
Contributor

Choose a reason for hiding this comment

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

As we are having a bunch of these if statements , we might try using a object to map type to path , also having source_type and type together make the function little complex , hence maybe be prone to bugs ( something to consider)

Copy link
Member

Choose a reason for hiding this comment

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

Sounds appropriate to me. @aabidsofi19 will you jump in with a code suggestion (make a commit to this PR/branch)?

if (filtersCatalogueCapability?.length > 0) setCanPublishFilter(true);
}
},
(err) => console.error(err)
Copy link
Contributor

Choose a reason for hiding this comment

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

can we do something more with the error .

Copy link
Member

Choose a reason for hiding this comment

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

Yes, please, @Chadha93. A toaster?

},
PUBLISH_CATALOG : {
name : "PUBLISH_CATALOG",
error_msg : "Failed to publish catalog"
Copy link
Member

Choose a reason for hiding this comment

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

What does this mean? "Failed to publish catalog"

Please rephrase.

Copy link
Member

Choose a reason for hiding this comment

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

Rephrase. Include filter name or filename as confirmation for the user.

},
UNPUBLISH_CATALOG : {
name : "PUBLISH_CATALOG",
error_msg : "Failed to publish catalog"
Copy link
Member

Choose a reason for hiding this comment

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

Rephrase. Include filter name or filename as confirmation for the user.

if (filtersCatalogueCapability?.length > 0) setCanPublishFilter(true);
}
},
(err) => console.error(err)
Copy link
Member

Choose a reason for hiding this comment

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

Yes, please, @Chadha93. A toaster?

{ credentials : "include", method : "DELETE", body : JSON.stringify({ "id" : filter?.id }) },
() => {
updateProgress({ showProgress : false });
enqueueSnackbar("Filter unpublished", {
Copy link
Member

Choose a reason for hiding this comment

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

What filter? Provide the name as confirmation to the user.

@Chadha93, remember when you had to go through the code base and make the notifications uniform by adding the name of the thing into the toaster message... ? Please uphold this convention.

@@ -224,9 +234,39 @@ function MesheryFilters({ updateProgress, enqueueSnackbar, closeSnackbar, user,
CLONE_FILTERS : {
name : "CLONE_FILTER",
error_msg : "Failed to clone filter file"
},
PUBLISH_CATALOG : {
name : "PUBLISH_CATALOG",
Copy link
Member

Choose a reason for hiding this comment

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

This name is a bit of a misnomer, no? Please rename accordingly.

return async () => {
let response = await modalRef.current.show({
title : `Unpublish Catalog item?`,
subtitle : `Are you sure you want to unpublish ${filter?.name}?`,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
subtitle : `Are you sure you want to unpublish ${filter?.name}?`,
subtitle : `Are you sure that you want to unpublish "${filter?.name}"?`,

{ credentials : "include", method : "POST", body : JSON.stringify(catalog_data) },
() => {
updateProgress({ showProgress : false });
enqueueSnackbar("Filter Published!", {
Copy link
Member

Choose a reason for hiding this comment

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

Why are you capitalizing "Published"? Please don't.

Also, please don't yell at the user! It's not nice, is it?

try {
downloadFile({ id, name, type : "filter" })
updateProgress({ showProgress : false });
enqueueSnackbar(`"${name}" Filter downloaded`, {
Copy link
Member

Choose a reason for hiding this comment

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

What filter?

}
});
} catch (e) {
console.error(e);
Copy link
Member

Choose a reason for hiding this comment

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

What are we doing when this happens?

@@ -12,6 +12,10 @@ export default function downloadFile({ id, type,name, source_type }) {
dataUri = `/api/pattern/download/${id}`;
}

if (type === "filter") {
Copy link
Member

Choose a reason for hiding this comment

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

Sounds appropriate to me. @aabidsofi19 will you jump in with a code suggestion (make a commit to this PR/branch)?

@leecalcote
Copy link
Member

Urgently needed. Merging. Please follow up on comments.

@leecalcote leecalcote merged commit 5b153a4 into meshery:master Jun 30, 2023
15 checks passed
@Chadha93
Copy link
Member Author

Urgently needed. Merging. Please follow up on comments.

Working on comments, I just checked the publish is breaking for filters, as Publish modal was not updated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/ui User Interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants