-
Notifications
You must be signed in to change notification settings - Fork 116
Feature/adding query md experimental support #923
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
Feature/adding query md experimental support #923
Conversation
@@ -33,3 +40,13 @@ export default function WorkflowQueriesResultJson(props: Props) { | |||
</styled.ViewContainer> | |||
); | |||
} | |||
|
|||
const isContentMarkdown = (content: any): content is Markdown => { |
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't write typescrypt, so feel free to let me know if you want me to learn how to use the type system more properly.
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 is correct, but with above type updates we wont need it any more
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: If we still need this helper, it should be moved to a separate file
@@ -1,7 +1,7 @@ | |||
import { | |||
type QueryJsonContent, | |||
type Props, | |||
} from '../workflow-queries-result-json.types'; | |||
} from '../workflow-queries-result.types'; | |||
|
|||
export default function getQueryJsonContent(props: Props): QueryJsonContent { |
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.
Json can be removed from this name also since it is now returning multiple types of contents.
src/views/workflow-queries/workflow-queries-result/helpers/get-query-json-content.ts
Outdated
Show resolved
Hide resolved
@@ -33,3 +40,13 @@ export default function WorkflowQueriesResultJson(props: Props) { | |||
</styled.ViewContainer> | |||
); | |||
} | |||
|
|||
const isContentMarkdown = (content: any): content is Markdown => { |
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 is correct, but with above type updates we wont need it any more
src/components/markdown/md.styles.ts
Outdated
alignSelf: 'stretch', | ||
flexDirection: 'row', | ||
alignItems: 'flex-start', | ||
padding: '30px', |
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 this markdown is in the components
directory which is intended for highly reusable components, we usually differ layout styles to the Usage (Pages). This allows components to be placed in different pages with different Styles (padding, background colors etc.).
In this case we can follow the how PrettyJSON
component is done. So style for text is done here and surrounding styles are added to WorkflowQueriesResult
@@ -55,6 +59,6 @@ function setup({ | |||
loading?: boolean; |
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.
we can update the tests to check if type is markdown
we show the Markdown Mock
and in case of JSON we show PrettyJson Mock
@@ -33,3 +40,13 @@ export default function WorkflowQueriesResultJson(props: Props) { | |||
</styled.ViewContainer> | |||
); | |||
} | |||
|
|||
const isContentMarkdown = (content: any): content is Markdown => { |
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: If we still need this helper, it should be moved to a separate file
@@ -8,7 +8,12 @@ export type Props = { | |||
loading?: boolean; | |||
}; | |||
|
|||
export type Markdown = { |
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.
Instead of adding new types of content on a case-by-case basis, we can have a generic "custom content" type and add a configuration that parses it based on what the "type" field is.
This is an experimental feature which allows a query to return preformatted data (in this instance markdown, though if it's useful, presumably other types as well).
This is still being tested, so it's not going to be super discoverable (and I have no idea what I'm doing on the frontend, so also will try and wall off these changes for a bit until we're more certain they'll be of use).
The idea is that a query should be able return a response struct like this:
for example:
where
cadenceResponseType
is expected to always be the fixed stringformattedData
andformat
refers to the mime-type classification.If it works, and if the format returned is supported, this can be rendered in the query UI. The react-md package in this instance takes care to sanitize the inputs since this would otherwise be an obvious XSS vector.
Testing: