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

Render markdown in description fields (fixes #153) #155

Merged
merged 5 commits into from
Oct 30, 2020

Conversation

alvesitalo
Copy link
Contributor

@alvesitalo alvesitalo commented Oct 23, 2020

This creates a markdown render component and adds a story.

Pull Request checklist

  • The pull request has a descriptive title (and a reference to an issue it
    fixes, if applicable)
  • All tests and linter checks are passing
  • The pull request is free of merge conflicts

Copy link
Contributor

@wlach wlach left a comment

Choose a reason for hiding this comment

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

This looks great, I actually don't have any feedback. :) Merging as-is.

Copy link
Contributor

@wlach wlach left a comment

Choose a reason for hiding this comment

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

Actually, sorry, there are some changes I'd request (it's been a long day). With regard to ping descriptions, we should also use this component on this page: https://github.com/mozilla/glean-dictionary/blob/main/src/pages/PingDetail.svelte

Also, I think we need to apply this to more than just pings. For example, metrics might have markdown in their descriptions as well. See, for example:

http://localhost:5000/#!/apps/fenix/metrics/metrics.search_count

src/components/PingDescription.svelte Outdated Show resolved Hide resolved
@alvesitalo
Copy link
Contributor Author

alvesitalo commented Oct 28, 2020

Pings and metrics are having markdown in their descriptions now. In addition, the markdown is being rendered inline, so pings will no longer have a <p> wrapping tag. 🎉

Copy link
Contributor

@wlach wlach left a comment

Choose a reason for hiding this comment

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

Code looks good now, thanks!

Thinking about this a bit more, I think we should generalize this to a Markdown component and use slots, instead of hardcoding a description field . That way you could do something like this just about anywhere:

<Markdown>
This my *markdown* text with a [link](https://mozilla.org)
</Markdown>

Do you mind making the modifications? After that, I'd be very happy to land.

@alvesitalo
Copy link
Contributor Author

I don't. I think It's a great idea.

@alvesitalo
Copy link
Contributor Author

Done! Markdown component is working, I just had to add a text var to work as a fallback for slot on StoryBook. Let me know if everything is OK.

@alvesitalo alvesitalo requested a review from wlach October 30, 2020 16:16
{/if}
<div class="raw-markdown hidden" bind:this={markdown}>
<slot>{text}</slot>
</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, it's a bit unfortunate that this adds a hidden div to the document:

image

I offhand don't know of a better way of doing this though, and your solution does work well, so am inclined to merge.

@wlach wlach merged commit e5dd308 into mozilla:main Oct 30, 2020
@wlach
Copy link
Contributor

wlach commented Oct 30, 2020

Awesome work @alvesitalo! Congratulations on getting this over line.

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.

2 participants