Skip to content
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

refactor(TextFormatter): create TextFormatter component TASK-996 #5257

Closed
wants to merge 1 commit into from

Conversation

pauloamorimbr
Copy link
Contributor

@pauloamorimbr pauloamorimbr commented Nov 12, 2024

🗒️ Checklist

  1. run linter locally
  2. update all related docs (API, README, inline, etc.), if any
  3. draft PR with a title <type>(<scope>)<!>: <title> TASK-1234
  4. tag PR: at least frontend or backend unless it's global
  5. fill in the template below and delete template comments
  6. review thyself: read the diff and repro the preview as written
  7. open PR & confirm that CI passes
  8. request reviewers, if needed
  9. delete this section before merging

👀 Preview steps

  • The component is available for visualization and tests in storybook

💭 Notes

A TextFormatter component is being added to support long strings formatted in a markdown-like fashion.
Such component is needed to support long sentences to be properly translated without having to break out words just to add simple styles like bold, italic or links.

  • The TextFormatter component suports an input of a text prop containing an string to be processed and generates the output based on the processing of the string.
  • Formatting can be nested if needed

const formattedParts: ReactNode[] = [];

for (const part of parts) {
if (part.startsWith('**')) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a style issue, but we generally like to avoid if/else chains in favor of multiple simple if statements:

if (x) {
    return a
}
if (y) {
    return b
}
return c

@jamesrkiger
Copy link
Contributor

Left one minor comment, but other than that lgtm.

@jamesrkiger jamesrkiger changed the title feat(organizations): Create TextFormatter component TASK-996 feat(organizations): create TextFormatter component TASK-996 Nov 13, 2024
@magicznyleszek
Copy link
Member

magicznyleszek commented Nov 13, 2024

@pauloamorimbr do we have plans to use it in all the places that currently go with dangerouslySetInnerHTML? I think there are around 11 of them (and not all apply).

The list of components that could use TextFormatter:

  • ActivityMessage
  • SharingForm (uses replaceBracketsWithLink)
  • TransferProjects
  • AccessDenied (uses replaceBracketsWithLink)
  • UniversalTable

Also do we have plans to replace our old way of handling links (replaceBracketsWithLink from textUtils.ts) with this new way? :) Not sure if these two are really compatible, as replaceBracketsWithLink has a bit different functionality (there is no URL in the translation string, e.g. foo [link] bar, FE code provides the link) - but I think at least we should acknowledge one and the other with a comment. I haven't looked too deep into this :P

@magicznyleszek
Copy link
Member

Also I think that this should be discussed with @JacquelineMorrissette (unless it already was) given the fact that we are kinda setting some new rules for how translation text is written.

@@ -0,0 +1,56 @@
import type {ReactNode} from 'react';

const regExpMatchParts = /[^*[\]]+|(\*\*[^*]*(?:\*[^*]+)*\*\*)|(\*[^*]+\*)|(\[.+?\]\(.+?\)({:.+})?)/g;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've used react-markdown before and suggest it. I like most the ability to override all components with custom implementation, that's useful especially with Link-s.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the need here was a simple subset of what markdown does I opted not to use a full library, not at first at least, so we wouldn't have to worry about unwanted markdown styles being accidentally inserted in string and also implementing it simply would avoid having another library linked to the project, but after talking to @Akuukis about this I ended up agreeing that using a library would simplify things here, mainly because there are libraries where we can opt for which marks we want to implement.

@pauloamorimbr pauloamorimbr changed the title feat(organizations): create TextFormatter component TASK-996 feat(TextFormatter): create TextFormatter component TASK-996 Nov 13, 2024
@pauloamorimbr pauloamorimbr changed the title feat(TextFormatter): create TextFormatter component TASK-996 refactor(TextFormatter): create TextFormatter component TASK-996 Nov 13, 2024
@pauloamorimbr
Copy link
Contributor Author

Also I think that this should be discussed with @JacquelineMorrissette (unless it already was) given the fact that we are kinda setting some new rules for how translation text is written.

I did talk to @JacquelineMorrissette about this. 👍🏻

@pauloamorimbr
Copy link
Contributor Author

@pauloamorimbr do we have plans to use it in all the places that currently go with dangerouslySetInnerHTML? I think there are around 11 of them (and not all apply).

The list of components that could use TextFormatter:

  • ActivityMessage
  • SharingForm (uses replaceBracketsWithLink)
  • TransferProjects
  • AccessDenied (uses replaceBracketsWithLink)
  • UniversalTable

Also do we have plans to replace our old way of handling links (replaceBracketsWithLink from textUtils.ts) with this new way? :) Not sure if these two are really compatible, as replaceBracketsWithLink has a bit different functionality (there is no URL in the translation string, e.g. foo [link] bar, FE code provides the link) - but I think at least we should acknowledge one and the other with a comment. I haven't looked too deep into this :P

I didn't think of migrating other stuff. The idea is to apply this to the changes I'm working on right now.
If you think we could benefit from applying the same to other pieces of code I believe we could create another task to handle that. WDYT?

Regarding replacing the replaceBracketsWithLink, using markdown allow us to have better text for translation. I've taken a look at it and it's not exactly the same. I believe it falls in the same answer as above. We could create a new task to refactor what's being done currently.
I didn't consider using HTML directly due to safety reasons and also because it's a less recognizable text for translatos...that's why I'm suggesting the new approach.

@pauloamorimbr
Copy link
Contributor Author

I've done tests with the react-markdown and it seems to work pretty well. For now I don't think we will benefit from wrapping it in our own component, so I will close this PR if no one disagrees in favor of just using the react-markdown on #5249 directly.

@pauloamorimbr pauloamorimbr deleted the task-996-add-text-formatter-component branch November 13, 2024 23:41
@magicznyleszek
Copy link
Member

@pauloamorimbr made a task for the refactor proposition: https://www.notion.so/kobotoolbox/Refactor-some-dangerouslySetInnerHTML-code-to-use-react-markdown-13e7e515f65480c5a95ec9b1a7a7477c?pvs=4

@pauloamorimbr
Copy link
Contributor Author

Thanks. I'm mentioning this in the other PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants