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 rendering / Normalized Html styling #395

Merged
merged 13 commits into from
Apr 11, 2020

Conversation

corydickson
Copy link
Contributor

@corydickson corydickson commented Jun 15, 2019

Addresses issue #386

I had to update the rollup config with a JSON plugin due to a failed installation. Let me know if you are able to reproduce.

Also since we are using jsx everywhere I don't think we'll need to sanitize the input.

@auto-assign auto-assign bot requested a review from bpierre June 15, 2019 17:39
@welcome
Copy link

welcome bot commented Jun 15, 2019

Thanks for opening this pull request! Someone will review it soon 🔍

@CLAassistant
Copy link

CLAassistant commented Jun 15, 2019

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@bpierre bpierre left a comment

Choose a reason for hiding this comment

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

Awesome, thank you @Gh1dra! 🎉

I left a few comments, let me know what you think! 🤗

rollup.config.js Outdated Show resolved Hide resolved
devbox/package-lock.json Outdated Show resolved Hide resolved
devbox/apps/MarkdownRenderer.js Outdated Show resolved Hide resolved
src/components/RenderedMarkdown/RenderedMarkdown.js Outdated Show resolved Hide resolved
src/components/RenderedMarkdown/RenderedMarkdown.js Outdated Show resolved Hide resolved
src/components/NormalizedHtml/NormalizedHtml.js Outdated Show resolved Hide resolved
devbox/apps/MarkdownRenderer.js Outdated Show resolved Hide resolved
src/components/RenderedMarkdown/RenderedMarkdown.js Outdated Show resolved Hide resolved
src/components/RenderedMarkdown/RenderedMarkdown.js Outdated Show resolved Hide resolved
src/components/RenderedMarkdown/RenderedMarkdown.js Outdated Show resolved Hide resolved
@corydickson
Copy link
Contributor Author

@bpierre @chadoh Thank you both for your review! Let me know if there is anything I didn't address from the original issue.

Currently, Travis is failing due to the rollup config: https://travis-ci.org/aragon/aragon-ui/jobs/553950501

Here is the new issue: #400

@stale stale bot added the abandoned label Aug 2, 2019
@bpierre bpierre removed the abandoned label Aug 2, 2019
@aragon aragon deleted a comment from stale bot Aug 2, 2019
@stale stale bot added the abandoned label Sep 7, 2019
@aragon aragon deleted a comment from stale bot Sep 7, 2019
@stale stale bot removed the abandoned label Sep 7, 2019
@stale stale bot added the abandoned label Oct 7, 2019
@aragon aragon deleted a comment from stale bot Oct 7, 2019
@stale stale bot removed the abandoned label Oct 7, 2019
@corydickson
Copy link
Contributor Author

@bpierre Is there anything I can do to help push this over the line? Are there any updates using the new design system that you'd like to see incorporated?

@bpierre
Copy link
Contributor

bpierre commented Oct 27, 2019

Hi @Gh1dra, sorry about the super long wait. We are going to review the last changes during the next week or so.

Are there any updates using the new design system that you'd like to see incorporated?

Ideally yes! Would you mind moving these changes on top of newstyle rather than master?

Some changes that will be needed for 1.0:

  • Replace theme by useTheme(), and make sure all the colors are coming from it (so that it’s ready for the dark theme).
  • Replace font by textStyle().
  • Use GU (e.g. ${2 * GU}px rather than 16px.
  • Replace SafeLink by Link.

Let me know if you have any question, I am also bpierre on https://aragon.chat :-)

@stale stale bot added the abandoned label Nov 26, 2019
@aragon aragon deleted a comment from stale bot Nov 26, 2019
@stale stale bot removed the abandoned label Nov 26, 2019
@stale stale bot added the abandoned label Dec 26, 2019
@aragon aragon deleted a comment from stale bot Dec 26, 2019
@stale stale bot removed the abandoned label Dec 26, 2019
@stale stale bot added the abandoned label Jan 25, 2020
@aragon aragon deleted a comment from stale bot Jan 25, 2020
@stale stale bot removed the abandoned label Jan 25, 2020
@corydickson
Copy link
Contributor Author

corydickson commented Jan 28, 2020

@bpierre Sorry for the delay, rollup is still giving me issues on my local machine but seems to be passing your build pipeline. Edit: I'm wondering why this rollup-json plugin is now suddenly a requirement... If this looks good I'll squash the prior commits and add @chadoh as an author

@bpierre bpierre mentioned this pull request Jan 30, 2020
@stale stale bot added the abandoned label Feb 27, 2020
@aragon aragon deleted a comment from stale bot Feb 27, 2020
@stale stale bot removed the abandoned label Feb 27, 2020
@stale stale bot added the abandoned label Mar 28, 2020
@aragon aragon deleted a comment from stale bot Mar 28, 2020
@stale stale bot removed the abandoned label Mar 28, 2020
It is lighter than react-markdown (5kb vs. 20kb), and doesn’t have the
bundling issues that react-markdown has.

Also:

- Fix the theme colors.
- Use Link.
- Tweak NormalizedHtml styles.
- Use aragonUI’s Checkbox.
It is lighter than react-markdown (5kb vs. 20kb), and doesn’t have the
bundling issues that react-markdown has.

Also:

- Fix the theme colors.
- Use Link.
- Tweak NormalizedHtml styles.
- Use aragonUI’s Checkbox.
@bpierre
Copy link
Contributor

bpierre commented Apr 6, 2020

Hey @corydickson, thanks for this, and sorry for the delay!

I added a few commits:

  • Move to markdown-to-jsx, which is lighter than react-markdown (5kb vs. 20kb), and doesn’t have the
    bundling issues that react-markdown has.
  • Fix the theme colors.
  • Use <Link /> and <Checkbox /> in the rendered markdown.
  • Tweak NormalizedHtml styles.
  • Add documentation.
  • Other tweaks, coding style, prop types etc.

I think it is ready to be merged soon :-)

@bpierre bpierre requested review from sohkai and Evalir April 6, 2020 19:13
@corydickson
Copy link
Contributor Author

@bpierre Thanks! Do you want to handle to rebase then? Want to be sure @chadoh gets authorship / green squares :)

@bpierre
Copy link
Contributor

bpierre commented Apr 7, 2020

@corydickson I only merged master in, using merge commits! For the autorship issue, did you want to squash your previous commits to make @chadoh the author? If that’s the case feel free to do so, I’ll wait for it before merging. Note that we always squash & merge our PRs, so all the commits here will appear as a single commit on master. Let me know!

@corydickson corydickson force-pushed the markdown-rendering branch 2 times, most recently from 42f91bd to ee752f4 Compare April 7, 2020 11:10
@corydickson
Copy link
Contributor Author

So when I try to rebase it makes me the author of all the child commits from 728553e onward... let's just add: Co-authored-by: Chad Ostrowski <hi@chadoh.com> at the end when you squash after review sorry for the confusion 😅

Copy link
Contributor

@sohkai sohkai left a comment

Choose a reason for hiding this comment

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

A couple comments and we're having an offline discussion about the unstyled prop, but otherwise, LGTM!

src/components/NormalizedHtml/README.md Outdated Show resolved Hide resolved
src/components/Markdown/Markdown.js Show resolved Hide resolved
src/components/Markdown/README.md Outdated Show resolved Hide resolved
This is now an internal component of Markdown.

Also:

- Replace unstyled by normalized.
- Let users extend styles with the className and style props.
- Minor tweaks.
@bpierre bpierre merged commit cf5c608 into aragon:master Apr 11, 2020
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