-
Notifications
You must be signed in to change notification settings - Fork 70
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
feat (O3-3946): Add support for markdown questions #355
base: main
Are you sure you want to change the base?
feat (O3-3946): Add support for markdown questions #355
Conversation
@Twiineenock could you please update your branch? |
<p className={styles.questionLabel}> | ||
<MarkdownWrapper markdown={question.label || question.value} /> | ||
</p> |
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 the text shown in markdown can be quite long, unlike a question label, I don't think we should show the entirety of it here. Maybe we could just say Markdown content
, or something along those lines?
@denniskigen thoughts?
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.
@NethmiRodrigo , we have come up with this:
If the markdown is very long, we show a small part of the markdown plus ellipsis to show that there is more 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.
Lovely! Thanks @Twiineenock!
@@ -0,0 +1,54 @@ | |||
import React from 'react'; |
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.
Love that you made this into a component of its own! Thank you!
<Select | ||
defaultValue={questionToEdit.questionOptions.rendering} | ||
onChange={(event: React.ChangeEvent<HTMLSelectElement>) => setFieldType(event.target.value as RenderType)} | ||
id="renderingType" | ||
invalidText={t('validFieldTypeRequired', 'A valid field type value is required')} | ||
labelText={t('renderingType', 'Rendering type')} | ||
required | ||
> |
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'd prefer to have the question label be the first input in the modal, it feels more intuitive.
import styles from './markdown-question.scss'; | ||
|
||
interface MarkdownQuestionProps { | ||
palceHolder?: string; |
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.
palceHolder?: string; | |
placeholder?: string; |
package.json
Outdated
"rehype-raw": "^7.0.0", | ||
"remark-gfm": "^4.0.0", |
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.
Are these libraries to support for html tags? If so, I'd rather we leave them out and not have support for HTML tags
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.
Because I'm concerned about XSS - https://www.npmjs.com/package/react-mde#xss-concerns
CCing @denniskigen
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 bad, I see we already support some tags in the form engine - https://github.com/openmrs/openmrs-esm-form-engine-lib/blob/810fd93961be3a38c3e3afe351996d676de65eec/src/components/inputs/markdown/markdown-wrapper.component.tsx#L9
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.
Are these libraries to support for html tags? If so, I'd rather we leave them out and not have support for HTML tags
Thanks @NethmiRodrigo for the Review,
We do need remark-gfm
if we are to have GitHub Flavored Markdown (GFM) e.g tables, task lists, strikethroughs, Autolinks e.t.c
And we need The rehypeRaw
plugin processes raw HTML embedded within Markdown content, allowing it to render as HTML elements in the final output.
Are there some of these formatting options we don't need so we can keep it as simple as possible?
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.
Lets limit the formatting options to only the ones that are currently supported by the react form engine. We don't need these libraries then right?
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.
Lets limit the formatting options to only the ones that are currently supported by the react form engine. We don't need these libraries then right?
We now have this implemented
…k/openmrs-esm-form-builder into feat/support-for-markdown-qns
Hi @NethmiRodrigo, @denniskigen, I'm currently trying to import the react-mde styles directly from the library instead of rewriting them. However, I noticed that our Webpack configuration is enabling CSS Modules globally, which is likely scoping the CSS classes to components, making them unavailable for global styles like those from Here’s the relevant part of the Webpack configuration: const cssLoader = {
loader: require.resolve('css-loader'),
options: {
modules: {
localIdentName: `${ident}__[name]__[local]___[hash:base64:5]`,
},
},
}; I suspect that this configuration is preventing the How can we tweak the Webpack config to ensure that CSS Modules are applied to our local styles, but exclude the styles from Thanks! |
I can't seem to figure out why the e2e tests are failing, they pass perfectly fine locally. When creating a question, the save button in the modal stays disabled for some reason. Pinging @denniskigen for help |
@Twiineenock about this, with help from @ibacher, I changed the webpack config to overwrite the css rule, which is basically |
@Twiineenock lets opt to leave the styling as you've done. Although for future work, we should make the markdown editor match the carbon design system - https://carbondesignsystem.com/patterns/text-toolbar-pattern/. All that's left is figuring out why the e2e test isn't working. I doubt it has anything to do with your changes because they pass locally |
Woow, great Idea @NethmiRodrigo , Is it better when its a separate work? |
@Twiineenock if you would like to do it in this PR, we can go with that approach. I would prefer we get this in and do that work incrementally, since this feature works perfectly fine as is. About the e2e test, could you please update the |
Alright we shall do that incrementally |
@@ -322,14 +325,16 @@ const AddQuestionModal: React.FC<AddQuestionModalProps> = ({ | |||
<ModalBody hasScrollingContent> | |||
<FormGroup legendText={''}> | |||
<Stack gap={5}> | |||
<TextInput | |||
{renderingType === 'markdown' ? <MarkdownQuestion onValueChange={setQuestionValue}/> : ( | |||
<TextInput | |||
id="questionLabel" | |||
labelText={<RequiredLabel isRequired={isQuestionRequired} text={t('questionLabel', 'Label')} t={t} />} | |||
placeholder={t('labelPlaceholder', 'e.g. Type of Anaesthesia')} | |||
value={questionLabel} | |||
onChange={(event: React.ChangeEvent<HTMLInputElement>) => setQuestionLabel(event.target.value)} | |||
required |
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.
Shall we remove the required attributes and change the label to not be required as well, since it is no longer so
id={questionToEdit.id} | ||
labelText={t('questionLabel', 'Label')} | ||
onChange={(event: React.ChangeEvent<HTMLInputElement>) => setQuestionLabel(event.target.value)} | ||
required |
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.
Ditto
Just a few final changes and we can merge this in @Twiineenock, could you also please update your branch? |
d79ef4e
to
6a4552b
Compare
src/types.ts
Outdated
@@ -117,7 +117,7 @@ export interface Section { | |||
export interface Question { | |||
id: string; | |||
label?: string; | |||
value?:string; | |||
value?: string; | |||
type: string; |
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 should make this optional since markdown content doesn't have a type
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.
And we should update this in form engine too
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.
@NethmiRodrigo , we have a PR here feat: FormField Type should be Optioanl #419 for this.
src/types.ts
Outdated
@@ -117,8 +117,8 @@ export interface Section { | |||
export interface Question { | |||
id: string; | |||
label?: string; | |||
value?:string; | |||
type: string; | |||
value?: string; |
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.
@Twiineenock I see value
is of type any
in form-engine and not string
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.
@NethmiRodrigo I had set the type to string because of this eslint
error Unexpected any. Specify a different type.eslint[@typescript-eslint/no-explicit-any](https://typescript-eslint.io/rules/no-explicit-any)
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.
Should we go for any
or string
?
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.
No worries! Added this // eslint-disable-next-line @typescript-eslint/no-explicit-any
inline comment to escape eslint
check for type any
when validating question value
attribute.
Requirements
Summary
Description
This pull request introduces support for creating markdown-based questions within the interactive form builder. Markdown questions are designed for scenarios where additional explanatory content is needed within the form, apart from standard question labels. This content can help users understand how to fill out fields, explain the meaning of specific fields, or add other context. Since these questions are meant for accessibility and enhanced guidance, they do not contain fields to fill out.
Implementation Details
Currently, there is markdown rendering support in the
openmrs-esm-form-engine-lib
using react-markdown, which renders markdown strings as HTML elements within the UI.To provide full markdown support with editing capabilities, this PR includes the following key components:
Key Changes
UI Enhancements:
Integration with
form-engine
:openmrs-esm-form-engine-lib
to seamlessly integrate the markdown editor with existing question types.react-markdown
within the form engine to convert markdown text into React-rendered HTML elements.Features Added in this PR
By implementing this feature, the form builder now offers greater flexibility and accessibility, allowing clients to incorporate rich text explanations and formatting within their forms.
Technical Note: The core of this markdown support relies on
react-markdown
for rendering andreact-mde
for user editing. These libraries handle markdown safely and ensure React compatibility.Links
Screenshots
Screancasts
Creating a new question:
creat.webm
Edit an existing Question
edit-qn.webm
Related Issue
https://openmrs.atlassian.net/browse/O3-3946
Other