feat(core/presentation): allow markdown in ValidationMessage #7510
Conversation
christopherthielen
requested review from
alanmquach,
erikmunson,
maggieneterval and
Jammy-Louie
| private parser: Parser = new Parser(); | ||
| private renderer: HtmlRenderer = new HtmlRenderer(); | ||
| export function Markdown(props: IMarkdownProps) { | ||
| const { message, tag: tagProp, className: classNameProp, ...rest } = props; |
There was a problem hiding this comment.
what about destructuring with a default value here:
tag = 'div'
or is the concern that tag prop could be null?
| @@ -0,0 +1,9 @@ | |||
| .Markdown { | |||
There was a problem hiding this comment.
Curious if the capitalization here is a convention in deck?
There was a problem hiding this comment.
Generally we apply capitalization like this when a few things are true:
- The purpose of the class is to select all instances of a specific component type, and nothing else
- The class sits on the root element of that component type
- The class name is exactly identical to the name of the component type
The third point kind of goes hand-in-hand with the capitalization.
There was a problem hiding this comment.
This is a convention that has sort of organically appeared. It's a simple way to scope CSS to a specific component.
See also:
or in raw css:
There was a problem hiding this comment.
Got it, thanks for the details.
| private renderer: HtmlRenderer = new HtmlRenderer(); | ||
| export function Markdown(props: IMarkdownProps) { | ||
| const { message, tag: tagProp, className: classNameProp, ...rest } = props; | ||
| const tag = tagProp || 'div'; |
There was a problem hiding this comment.
Another option is to use a default prop for tag instead of aliasing then reassigning.
| const parsed = this.parser.parse(message.toString()); | ||
| const rendered = this.renderer.render(parsed); | ||
| restProps.dangerouslySetInnerHTML = { __html: DOMPurify.sanitize(rendered) }; | ||
| const restProps = rest as React.DOMAttributes<any>; |
There was a problem hiding this comment.
this makes me wish there was a nice way to describe IMarkdownProps as a union of React.DOMAttributes<any> and the other fields in a way where using ...rest would automatically let the type system narrow back down to React.DOMAttributes<any>. Oh well.
| @@ -43,7 +46,7 @@ export const ValidationMessage = (props: IValidationMessageProps) => { | |||
| return ( | |||
| <div className={`ValidationMessage ${containerClassName || containerClassNames[type] || ''}`}> | |||
| {showIcon && <i className={iconClassName || iconClassNames[type] || ''} />} | |||
| <div className="message">{message}</div> | |||
| <div className="message">{isString(message) ? <Markdown message={message} /> : { message }}</div> | |||
There was a problem hiding this comment.
I think something about this syntax got me confused. can you help me figure out what { message } is doing in this case? the way i'm reading it is that if message is anything other than a string, this will take the value of the message variable and make it the value of a message field inside an object, which will then get given to react to render. what am I missing?
There was a problem hiding this comment.
yes, this is a whoops... nice catch!
christopherthielen
force-pushed
the
use-markdown-in-validation-message
branch
from
119d0f8
to
30eafb0
Compare
This renders markdown in the
ValidationMessagecomponent