Skip to content

Conversation

@gnapse
Copy link
Contributor

@gnapse gnapse commented Feb 17, 2023

Reference

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

  • Added tests for bugs / new features
  • Updated docs (storybooks, readme)
  • Executed npm run validate and made sure no errors / warnings were shown
  • Described changes in CHANGELOG.md
  • Bumped version in package.json and package-lock.json (npm --no-git-tag-version version <major|minor|patch>) ref
  • 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.

@gnapse gnapse requested a review from a team February 17, 2023 18:34
@gnapse gnapse self-assigned this Feb 17, 2023
@gnapse gnapse requested review from Bloomca and removed request for a team February 17, 2023 18:34
Comment on lines 45 to 57
.prose > :first-child,
.prose :global(.ProseMirror > *:first-child) {
margin-top: 0;
}

.prose > :last-child,
.prose :global(.ProseMirror > *:last-child) {
margin-bottom: 0;
}

.prose :global(br.ProseMirror-trailingBreak:only-child) {
font-size: inherit;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These hardcoded references to a ProseMirror class name is something I want to figure out next week, maybe with the help of Ricardo, how we can make this work seamlessly without Reactist having to have knowledge about it.

Copy link
Member

@rfgamaral rfgamaral Feb 20, 2023

Choose a reason for hiding this comment

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

Do you want to discuss this here or over Twist? Perhaps in a sync fashion? Either way is fine with me :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A sync video call should be helpful and allow us to move faster. I'll ping you on Twist to coordinate.

Copy link
Contributor

@Bloomca Bloomca left a comment

Choose a reason for hiding this comment

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

The component looks good and the API seems reasonable. I am going to be honest, I didn't notice the difference between light/dark mode typography, but that's expected with such slight tweaks.

@gnapse gnapse force-pushed the ernesto/prose-component branch from c94d31f to fe9965d Compare February 20, 2023 17:17
@rfgamaral
Copy link
Member

rfgamaral commented Feb 20, 2023

@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 gnapse force-pushed the ernesto/prose-component branch from fe9965d to 66a59db Compare February 20, 2023 17:21
@gnapse
Copy link
Contributor Author

gnapse commented Feb 20, 2023

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.

@gnapse gnapse force-pushed the ernesto/prose-component branch from 66a59db to 50c2fb9 Compare February 20, 2023 17:33
@rfgamaral
Copy link
Member

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
Copy link
Contributor Author

gnapse commented Feb 20, 2023

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
Copy link
Member

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 😅

@gnapse gnapse force-pushed the ernesto/prose-component branch 2 times, most recently from dd96fec to 931b120 Compare February 21, 2023 20:04
@gnapse gnapse force-pushed the ernesto/prose-component branch from 4f8eb94 to b2c3a82 Compare February 28, 2023 18:18
@gnapse gnapse merged commit c79eaf8 into main Feb 28, 2023
@gnapse gnapse deleted the ernesto/prose-component branch February 28, 2023 18:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants