-
Notifications
You must be signed in to change notification settings - Fork 20
feat(#62): markdown support for labels, hints, and constraint messages #511
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
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: ebb6434 The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
span, | ||
label { | ||
font-weight: normal; | ||
} |
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'm not sure why this was in the reset in the first place and I couldn't find anything that changed when I removed this. It's needed because if the markdown sets something as bold and then there's an inner span/p you don't expect it to be overwritten by this css again.
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.
It’s probably meant to fix an issue when running Web Forms within Central. This reset file was mainly added to reduce overrides from Central's CSS. Hopefully, we can make Web Forms a standalone app soon and avoid these kinds of issues altogether.
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.
Good thinking! I've had a look in central and it still seems to work. In ControlLabel.vue we have label: { font-weight: 400}
which over-rights the bolding from central without the need for the reset. I wonder if the reset was added first, and then we specified the weight again at a later date making the reset unnecessary. An alternative would be for the reset to use inherit
instead of normal
but I unless I find something broken I'd rather remove it altogether.
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.
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.
Good find! I'm going to do more extensive testing inside Central when I deploy to the test server. In the meantime I've fixed this by using inherit
which overrides the Central bootstrap but not the weight when nested inside a <strong>
.
span,
label {
font-weight: inherit;
}
@latin-panda Review please! It's a fairly big change and much more complex than I'd like - feedback welcome! |
My todo list...
|
Great! I plan to review and test this later today and will look for additional forms to test. |
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.
Clear and easy to follow ✨
I’m still testing, but I’m adding some comments in the meantime.
span, | ||
label { | ||
font-weight: normal; | ||
} |
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.
It’s probably meant to fix an issue when running Web Forms within Central. This reset file was mainly added to reduce overrides from Central's CSS. Hopefully, we can make Web Forms a standalone app soon and avoid these kinds of issues altogether.
}, | ||
} as AnyInputNode; | ||
|
||
describe('ControlLabel', () => { |
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 feel this test has lost a lot of its value. It’s probably better to just have the E2E test asserting the labels and maintain only that one instead (and remove this). What do you think?
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.
just have the E2E test asserting the labels
Does this test already exist? Or are you thinking once the select one from map work is merged I write an e2e test to take a screenshot which checks for both markdown formatting and the required styling?
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.
There are tests already asserting that the label text is displayed, but not for "required" or other visual elements; we would need to create those tests.
if (!entries.length) { | ||
return; | ||
} | ||
return Object.fromEntries(entries) as StyleProperty; |
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.
Ah, the object could be built within the earlier loops to avoid that extra bit of processing.
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.
That was my initial attempt but I couldn't get the TS compiler to allow it. Because StyleProperty has readonly properties I couldn't set them one at a time in the loop. The way the engine usually does it would be to hand off all the parsing to the constructor which can set the properties before init completes, but that's a bit too much OO for me. There's probably some clever pattern I'm missing that's more idiomatic - any ideas?
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.
Yes, I can see the issue, as long as performance is good, this should be fine and still clear.
I reviewed the code again, and it looks good! I also tested it with some forms but didn't do a full regression. I found a few issues: 1.. Central's bootstrap is overriding select's options 2.. The text's line breaks aren't effective anymore; this is something reported by the forum before that we fixed here 3.. Is this 4.. This form displays more styles in Collect than this branch deployed in Central 5.. The image isn't displayed in the label Note this form combines markdown titles with emojis to increase the size of the emoji, to test this forum post: https://forum.getodk.org/t/large-single-character-emoji-text-display/56875 |
Thanks. Fixed (more details in PR).
Ooh yes. So the previous fix was to use Also I've edited the "all question types" xml so the "version" information is in an html list rather than attempting to manually indent it.
It's not part of this work, and I'm not sure what it should render. The current rendering is consistent with Enketo. Issue raised: #520
I had configured the DOMPurify to be highly restrictive, and only allow for elements which weren't already covered by markdown, ie: for a heading use
This is also a problem in
This is working in this branch, but because it's a |
packages/common/src/fixtures/preview-service/xforms/all-question-types-v2024091201-3.xml
Show resolved
Hide resolved
Interesting! The ![]() When you click the print button, it generates a QR code. ![]() |
35f83a5
to
25901c5
Compare
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.
<template> | ||
<div v-if="question.currentState.hint" class="hint"> | ||
{{ question.currentState.hint?.asString }} | ||
<MarkdownBlock v-for="(elem, index) in question.currentState.hint?.formatted" :key="index" :elem="elem" /> |
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.
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 propose we leave this as is - the Central team have decided that anything wider than 77ch is not readable. In this case it doesn't actually break web-forms and if they want to change it it can be reset on their side. If we start including overrides for everything any customer may have set in the parent context we'll end up with the world's biggest reset file (eg: we'd also have to do something about the following line p:lang(ja), p:lang(zh) { max-width: 54ch; }
). What do you think?
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.
Okay, in WF standalone, the text is fully expanded. This only affects WF in Central.
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 might get some issues reported, in that case, we'll coordinate with Central team to fix those css conflicts.
span, | ||
label { | ||
font-weight: normal; | ||
font-weight: inherit; |
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.
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.
Yes 😭
So the reset wasn't really working - even for the question labels we had specific weight override in ControlLabel.vue. I've added the same in select-options.scss
.
|
||
label, | ||
.hint { | ||
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.
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.
Oof. I'm not sure why this changed - I would expect it to be broken in main
too. Bottom line is Central defines a .label { color: white }
and we don't override it either in main
or this branch. Fixed by overriding.
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.
Hmm, I wonder if it's some new code on their affecting Web Forms.
export interface StyleProperty { | ||
readonly color: string | undefined; | ||
readonly 'font-family': string | undefined; | ||
readonly 'text-align': 'center' | 'left' | 'right' | undefined; |
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.
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 caused by the max-width: 77ch
as above. The reason it doesn't line up with the label is the font-size in the label is bigger and therefore the max-width
is wider. As above, I propose leaving this for Central to define whatever max-width they want.
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.
FYI @srujner
<span v-else-if="elem.role === 'html'" v-html="purify(elem)" /> | ||
|
||
<!-- link --> | ||
<a v-else-if="elem.role === 'parent' && elem.elementName === 'a'" :href="getUrl(elem)" target="_blank"> |
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.
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.
Yes precisely. I wanted to avoid actually parsing the HTML and just sanitizing it which means we're limited about what changes we can make to it. In this case, the person building the form would have to specify the target
attribute. Is that ok?
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.
Okay 👍 that's reasonable
<span>{{ props.question.currentState.label?.asString }}</span> | ||
<label :for="question.nodeId" :class="{ required: question.currentState.required }"> | ||
<MarkdownBlock v-for="(elem, index) in text" :key="index" :elem="elem" /> | ||
<div v-if="image || video || audio" class="media-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.
I think this Label's image should be left-aligned - @alyblenkin is that correct?
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.
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 like to put this out of scope, because it may require design input separate testing, etc. Should I remove the image handling code that I added pending dev on the separate issue, or do you think I leave it in there because it's better than nothing?
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 it's better to leave this code as is, showing an image is preferable to showing nothing. Can you please create a ticket to style it and tag Aly for input? :)
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.
Will do.
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.
Issue raised: #525
I've tested Android. Unfortunately I don't have access to an iPhone. |
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.
Exciting to have this ready. Thanks! 🤩
@latin-panda Sorry to bother you again, but Szymon sent through the first of the QA forms and I found another issue, which led me down a rabbit hole of more markdown in more places! Would you mind reviewing the last 3 commits as well? |
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.
panelClass += ' no-buttons'; | ||
} | ||
const getSelectedLabels = () => { |
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 could be a Vue's computed.
Ref: https://vuejs.org/guide/essentials/computed#computed-caching-vs-methods
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.
Fixed. Thanks. I need to read up more about vue...
}); | ||
}); | ||
const getSelectedLabel = () => { |
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.
Same feedback here
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.
Fixed.
@change="$emit('change')" | ||
/> | ||
> | ||
<template #option="slotProps"> |
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.
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.
Thanks for finding this. It turns out it needs a string to work. I've fixed this by passing the asString
value in as the option-label
.
<MarkdownBlock v-for="(elem, index) in slotProps.option.label.formatted" :key="index" :elem="elem" /> | ||
</template> | ||
<template #value> | ||
<MarkdownBlock v-for="(elem, index) in getSelectedLabel()" :key="index" :elem="elem" /> |
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.
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.
Ooh, yeah. I've updated the #value
template so it adds the comma in between. With options that don't use markdown it looks identical to before. With markdown it's got a bit more padding just because of the whitespace between HTML tags. I think it's ok, and difficult to manage without knowing what HTML they've included...


@srujner Collect displays selections as a comma-separated list, which better conveys what was selected rather than how many items were chosen. ![]() |
I think not. Let's leave it for now and revisit it once we have a use case. Because you can include structured elements it could break out of the list structure really easily... |
Closes #62 #198
I have verified this PR works in these browsers (latest versions):
What else has been done to verify that this works as intended?
Automated tests
Why is this the best possible solution? Were any other approaches considered?
How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?
Markdown processing runs automatically on all labels, so forms that include markdown styling accidentally will look different, for example, questions starting with a number will now be rendered as an ordered list which means they will be indented when they weren't before.
Do we need any specific form for testing your changes? If so, please attach one.
From this branch:
/packages/common/src/fixtures/notes/3-notes-with-markdown.xml
What's changed