-
Notifications
You must be signed in to change notification settings - Fork 1
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
[Cortex Scaffolder] Plugins Confluence Plugin #23
base: master
Are you sure you want to change the base?
Conversation
…each test, add reset of fetchmock data to beginWith clause. Exception handling to PageContent.tsx, fix App tests to load page content.
This reverts commit 91e33a9.
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.
Could you please replace this boilerplate README with something that describes the functions and benefits of the plugin as well as how to configure it, and ideally includes a screenshot of 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.
This can be deleted
import { PluginProvider } from "@cortexapps/plugin-core/components"; | ||
import "../baseStyles.css"; | ||
import ErrorBoundary from "./ErrorBoundary"; | ||
// import Content from "./Content" |
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.
// import Content from "./Content" |
{isNil(entityPage) ? ( | ||
<Box backgroundColor="light" padding={3} borderRadius={2}> | ||
<Text> | ||
We could not find any Confluence Page associated with this entity |
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.
nit:
We could not find any Confluence Page associated with this entity | |
We could not find any Confluence page associated with this entity |
}); | ||
}); | ||
|
||
it("Shows a page if page is found", async () => { |
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.
nit: these should read and be capitalized like sentences starting with "it"
it("Shows a page if page is found", async () => { | |
it("shows a page if page is found", async () => { |
import { getEntityYaml } from "../api/Cortex"; | ||
import { getConfluenceDetailsFromEntity } from "../lib/parseEntity"; | ||
|
||
const baseJIRAUrl = "https://cortex-se-test.atlassian.net"; |
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.
Updating this should be in the plugin's README as instructions for users as to how to point it at their own Jira/Confluence instance. Also, shouldn't this be
const baseJIRAUrl = "https://cortex-se-test.atlassian.net"; | |
const baseConfluenceUrl = "https://cortex-se-test.atlassian.net"; |
?
Lastly, Jira hasn't been stylized in all caps since 2017 😛
|
||
useEffect(() => { | ||
const fetchEntityYaml = async (): Promise<void> => { | ||
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion |
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.
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion |
setEntityPage(pageID?.pageID); | ||
const jiraURL = | ||
baseJIRAUrl + | ||
"/wiki/rest/api/content/" + | ||
// eslint-disable-next-line @typescript-eslint/restrict-plus-operands | ||
pageID?.pageID + |
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.
eslint is making a valid complaint here saying that pageID?.pageID
can be undefined
, in which case this will evaluate to the string "undefined"
. I think it would be cleanest to handle this scenario explicitly
setEntityPage(pageID?.pageID); | |
const jiraURL = | |
baseJIRAUrl + | |
"/wiki/rest/api/content/" + | |
// eslint-disable-next-line @typescript-eslint/restrict-plus-operands | |
pageID?.pageID + | |
if (!pageID?.pageID) { | |
throw new Error('No Confluence details for entity') | |
} | |
setEntityPage(pageID.pageID); | |
const jiraURL = `${baseJIRAUrl}/wiki/rest/api/content/${pageID.pageID}?expand=body.view`; |
Also, we can use string interpolation to make the URL building a bit cleaner
setPageContent(contentJSON.body.view.value); | ||
setPageTitle(contentJSON.title); | ||
} catch (e) { | ||
console.error("Error fetching Confluence page: ", e); |
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 a better UX would be to show an error in the HTML instead of having them see a blank screen and have to open up dev tools to parse what's happening.
<script | ||
defer | ||
type="text/javascript" | ||
src="https://cdnjs.cloudflare.com/ajax/libs/iframe-resizer/4.3.7/iframeResizer.contentWindow.js" |
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 updated the cookiecutter template since this was created 🙂. This should now be
src="https://cdnjs.cloudflare.com/ajax/libs/iframe-resizer/4.3.7/iframeResizer.contentWindow.js" | |
src="https://cdn.jsdelivr.net/npm/@iframe-resizer/child" |
Created by Cortex