feat: New `Prose` component to style free-form HTML content consistently

gnapse commented 1 year ago

Reference

https://twist.com/a/1585/ch/1293/t/4160768/c/81948654
Will help with https://github.com/Doist/Issues/issues/9444 (once this new Reactist releases is incorporated into Todoist)

Short description

A component to render free-form HTML and style it consistently. Mainly for rendering the markdown-generated content in our apps (both Twist and Todoist).

Test plan

I will thoroughly check first if this is easily adaptable to replace the markdown styles in Twist and Todoist before even merging it. I will also coordinate with designers to check the results in actual live examples.

Regardless of that, to the main PR reviewer, feel free to run storybooks and check things out.

PR Checklist

[x] Added tests for bugs / new features
[x] Updated docs (storybooks, readme)
[x] Executed npm run validate and made sure no errors / warnings were shown
[x] Described changes in CHANGELOG.md
[x] Bumped version in package.json and package-lock.json (npm --no-git-tag-version version <major|minor|patch>) ref
[x] Updated all static build artifacts (npm run build-all)
[ ] Reviewed and approved Chromatic visual regression tests in CI

Versioning

New minor release. Probably v20.1.0.

rfgamaral commented 1 year ago

@gnapse I've only got back, so please excuse me if I'm missing context for this, but by having this Prose component, aren't we decoupling the styling from the rendering (i.e. MarkdownContent component on the apps, which in turn uses ReactMarkdown)? I feel like we shouldn't decouple the renderer from the styling. I'm sure you have some thoughts on this, and I would love to hear your reasoning.

gnapse commented 1 year ago

aren't we decoupling the styling from the rendering

I do not understand what's the concern exactly. Isn't the renderer going to generate HTML anyway? This is just syling the HTML in a standardized way.

If you're concerned by anything non-standard that the renderer might output (e.g. a user mention) the corresponding draft PR in the Todoist web app shows how this can be dealt with in the app's code.

Or let me know if the concern is about something else that I'm missing.

rfgamaral commented 1 year ago

Or let me know if the concern is about something else that I'm missing.

No, not really, it's just that to me, it feels strange to have the renderer component in one place (the apps), and the styling in another place (reactist). I feel like they should be coupled somehow.

I guess my main concern is related to the user mention example you gave, because this Prose component will only take care of styling for standard HTML elements, and everything else is handled by the app, correct? I'm wondering if this Prose component shouldn't take care of the styling for all elements through CSS variables? In other words, do for user mentions (and other things) what you did for code, quote and links (ref), or is that not a good idea?

gnapse commented 1 year ago

I see no reason why the styling needs to be coupled to the markdown-to-html generator.

And I don't see a reason for the higher-level prose styles to be concerned with app specifics. Otherwise, it'd need to be aware of anything new that a client app might decide to add that's custom.

The client app will have to add some attributes to any custom thing, in order to style it differently. For instance, if the renderer outputs a mention, and that mention is a link (<a href="…"/>) element, if the app wants those particular types of links to be rendered differently than how Prose does it, they can still do it, by adding a custom class name to those elements at rendering time, and adding the custom CSS for that class name.

The advantage of having the non-custom general stylings here is that we standardize how it's styled. The idea came from when the design team just requested that we leveraged the styles from Twist's markdown-generated content in Todoist. The alternative solution would've been to copy the styles from Twist's code to Todoist, but then, in the future, if these styles change, we need to change them twice.

Anyway, I'm open for more discussion, and I may ask the question to the rest of the team to see what we collectively think.

rfgamaral commented 1 year ago

I see no reason why the styling needs to be coupled to the markdown-to-html generator.

I don't have any compelling arguments either. At first, it felt like they belong together, but I agree with your reasoning that they probably shouldn't be coupled together. Feel free to open the discussion to the rest of the team if you think it's a discussion worth having, if not, just ignore what I said 😅

Doist / reactist