Conversation
demiankatz
left a comment
demiankatz
left a comment
There was a problem hiding this comment.
Thanks, @ThoWagen, this makes sense to me, but see below for some minor comments.
| /** | ||
| * Apply encoding to the content based on the provided content type. | ||
| * | ||
| * @param string $contentType Content type |
There was a problem hiding this comment.
Should we list legal values here?
| * @param string $contentType Content type | |
| * @param string $contentType Content type (text, html, or markdown) |
| * @param Markdown $markdownHelper Markdown view helper | ||
| */ | ||
| protected $contextHelper; | ||
| public function __construct( |
There was a problem hiding this comment.
We should probably target this branch against dev-12.0 due to the BC-breaking changes to this helper... unless you feel strongly that we should roll this out early with a changelog warning. (I could be persuaded either way; I think it's relatively unlikely that anyone has locally extended the helper, but it's good to be fairly strict about semantic versioning when we can be).
| $content = ($this->escapeHtml)($content); | ||
| $content = $this->contentHelper->handleContentType( | ||
| $notice['contentType'] ?? 'text', | ||
| $this->translate($content) |
There was a problem hiding this comment.
Is it possible there might be scenarios where we would not want to translate the content? (Certainly, passing a big chunk of markdown as a translation key isn't going to work -- though it shouldn't cause harm either). Might we want a config setting to explicitly toggle translation, or is that overkill?
| 'component' => 'VuFind\View\Helper\Root\Component', | ||
| 'config' => 'VuFind\View\Helper\Root\Config', | ||
| 'content' => 'VuFind\View\Helper\Root\Content', | ||
| 'content' => \VuFind\View\Helper\Root\Content::class, |
There was a problem hiding this comment.
If we're going to make this change, maybe it would be best to do a separate PR to just universally adjust the style, rather than doing it piecemeal. I think @LuomaJuha was thinking of such a thing at one point, though I'm not sure if it's still on his to-do list somewhere.
2 participants
This PR adds the option for notices to allow HTML or Markdown.
It also makes all messages translatable using the normal language files.