LIF 612 - Add support for Flourish graphics #549
Draft
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
manasiSantFT
changed the title
joejeet
reviewed
Jul 23, 2025
|
|
||
|
|
||
| if(isFlourishElement) { | ||
| const match = elementSrc.match(/\/visualisation\/(\d+)\//); |
There was a problem hiding this comment.
Comment: What is the value of elementSrc?
Instead of Regex, we can use the built-in URL parser and then apply a focused regex only to the pathname. Regexes are fragile in case of matching with URL paths, as they will not cover the URL variations:
// Parse the URL
const urlObj = new URL(elementSrc);
joejeet
reviewed
Jul 23, 2025
| let isFlourishElement = false; | ||
| // identify flourish element | ||
| const elementSrc = el.getAttribute('src'); | ||
| if(elementSrc?.includes('public.flourish.studio/')) { |
There was a problem hiding this comment.
Comment/suggestion: I'll consider a more functional approach instead of imperative style when making changes in the DOM. We could simplify this to something like:
// Determine if it is a Flourish Element
const isFlourishElement = elementSrc?.includes('public.flourish.studio/');
// Determine image type
const imageType = isFlourishElement
? 'graphic'
: el.getAttribute('data-image-type');
This reduces multiple declarations and less to maintain.
joejeet
reviewed
Jul 23, 2025
| const RE_BAD_CHARS = /[^A-Za-z0-9_]/gm; | ||
| const RE_SPACE = /\s/gm; | ||
|
|
||
| function extractFourishEmbeds(contentHTMLBody) { |
joejeet
reviewed
Jul 23, 2025
| const flourishEmbeds = []; | ||
| let match; | ||
|
|
||
| while ((match = flourishIdRegex.exec(contentHTMLBody)) !== null) { |
There was a problem hiding this comment.
Comment: Regex.exec has a state inside it, explained here. Maybe we can use a simpler approach which doesn't involve any states.
Example:
for (const match of contentHTMLBody.matchAll(regex)) {
// match[1] will be captured ID I believe
ids2.push(match[1]);
}
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR introduces a temporary solution to support Flourish graphics in articles by ensuring they are included in the
content.embedsarray, even when not embedded.Ideally, Flourish graphics should be parsed and included in the
content.embedssection. Currently, they are not, which affects downstream rendering. This change bridges that gap until a more robust solution is implemented.Code changes
server/lib/enrich/article.jsserver/lib/builders/document-builder.jsremoveNonSyndicatableImagesto preserve Flourish content and prevent it from being removed inadvertently.Ticket
https://financialtimes.atlassian.net/browse/LIF-612