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

markdown preview #818

Merged
merged 29 commits into from
Mar 23, 2021
Merged

markdown preview #818

merged 29 commits into from
Mar 23, 2021

Conversation

tanmoyAtb
Copy link
Contributor

@tanmoyAtb tanmoyAtb commented Mar 12, 2021

closes #795

There are many plugins that could bee used with markdowns! However this minimalistic approach should be good enough for previews !

@render
Copy link

render bot commented Mar 12, 2021

@render
Copy link

render bot commented Mar 12, 2021

@render
Copy link

render bot commented Mar 12, 2021

@tanmoyAtb tanmoyAtb requested review from FSM1, RyRy79261 and Tbaut March 16, 2021 11:47
@tanmoyAtb tanmoyAtb marked this pull request as ready for review March 16, 2021 11:48
@Tbaut Tbaut added the Status: Review Needed 👀 Added to PRs when they need more review label Mar 16, 2021
@FSM1 FSM1 self-requested a review March 16, 2021 17:01
Copy link
Contributor

@FSM1 FSM1 left a comment

Choose a reason for hiding this comment

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

Great work, thanks Tanmoy 🎉

@RyRy79261
Copy link
Contributor

RyRy79261 commented Mar 16, 2021

@tanmoyAtb there doens't seem to be any markdown styling applied, besides the H1 (#) tags:

# Test

## Testing 

### Raspberry souffle 

### Ingredients
#### For the Raspberry Puree:

- 12 ounces frozen raspberries, thawed

#### For the Soufflé:

- 4 egg yolks
- 2 1/2 ounces granulated sugar
- 1.5 tablespoons cornstarch
- 4 ounces raspberry puree (see above)
- juice of one lemon
- 4 egg whites, room temperature
- pinch of cream of tartar
- pinch of salt
- 1 ounce granulated sugar
- powdered sugar, for dusting
- fresh raspberries, for garnish

@Tbaut Tbaut added Status: Changes Requested ❌ Added to PRs that require further changes from the contributor. and removed Status: Review Needed 👀 Added to PRs when they need more review labels Mar 18, 2021
@tanmoyAtb tanmoyAtb added the Status: Do Not Merge 🛑 Added to PRs that are not allowed to be merged. label Mar 19, 2021
@tanmoyAtb tanmoyAtb removed Status: Do Not Merge 🛑 Added to PRs that are not allowed to be merged. Status: Changes Requested ❌ Added to PRs that require further changes from the contributor. labels Mar 23, 2021
@Tbaut Tbaut added the Status: Review Needed 👀 Added to PRs when they need more review label Mar 23, 2021
Copy link
Collaborator

@Tbaut Tbaut left a comment

Choose a reason for hiding this comment

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

Working well apart for the code blocks that don't appear in a code block

I tested with the following
image

and it renders
image

const classes = useStyles()

useEffect(() => {
const getContentText = async () => {
Copy link
Collaborator

@Tbaut Tbaut Mar 23, 2021

Choose a reason for hiding this comment

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

Just a comment to share my thoughts, it's no big deal, but I've seen this form used widely across Files.

I think we should rather use the Promise.then in a hook, and handle errors accordingly (if this fails, we'll throw the app with an unhandeld exception). Async/await makes a lot of sense if we have many such async calls. It should then be wrapped in try/catch which makes it pretty verbose.

For instance here I'd suggest:

contents.text()
  .then((text) => setContentText(text))
  .catch(console.error)

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree that the .then(...).catch(...) is quite a bit more concise for simple handlers like this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated to then catch in both text and markdown preview !

@tanmoyAtb
Copy link
Contributor Author

Working well apart for the code blocks that don't appear in a code block

I tested with the following
image

and it renders
image

@Tbaut

The code blocks are working on my side

Screenshot 2021-03-23 at 8 46 29 PM

This is the md text

# Test

## Testing 

```js
const a = "code"

Copy link
Collaborator

@Tbaut Tbaut left a comment

Choose a reason for hiding this comment

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

The code is indeed in a <code> block. The font for me is font-family: monospace, monospace; which basically looks like the text font. Having no background color also reinforces the feeling. In any case it's not really a bug, we can make this better if the need appears.

@tanmoyAtb tanmoyAtb merged commit e36293a into dev Mar 23, 2021
@tanmoyAtb tanmoyAtb deleted the feat/markdown-preview-795 branch March 23, 2021 15:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Review Needed 👀 Added to PRs when they need more review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Markdown file preview
4 participants