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

WIP: Add markdown_editor component #2597

Closed
wants to merge 2 commits into from
Closed

Conversation

i-a-n
Copy link
Contributor

@i-a-n i-a-n commented Dec 5, 2019

Summary

Hi folks, long time user, first time contributor here. As mentioned in issue #1491 , I've been advised to start a PR with the work I've done thus far on a markdown editor for EUI.

euiMarkdownEditor

This is the result of work I've done for the Dream Machine project recently.

Now there's a lot left to do here, which I don't really have the immediate experience or time to accomplish efficiently, but I hope this will at least be a good starting point for the EUI team to run with. Here are some things even I know need some work, as I open this PR:

  • The i18n game is weak, and likely needs to be revamped
  • There are currently no meaningful tests
  • The state of the component is stored in the component itself. I assume we'll want that to be lifted up somehow.
  • The prettier task complains about a line that, if I "fix" it, makes TypeScript complain. Not sure how to resolve at present.
  • Currently, very "weakly" typed. There are more than one any & @ts-ignores present.
  • buliding fails with a small error about missing types for a library I added (called showdown)
  • Likely in need of a design review

But, in any case, I hope this is a helpful start. Please feel free to grab me on Slack (@ ian.moersen) if you want to talk in real-time!

/cc @ajangus @CBlaisure

Checklist

  • Checked in dark mode
  • Checked in mobile
  • Checked in IE11 and Firefox
  • Props have proper autodocs
  • Added documentation examples
  • Added or updated jest tests
  • Checked for breaking changes and labeled appropriately
  • Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

@i-a-n i-a-n requested review from snide and pickypg December 5, 2019 23:13
@snide
Copy link
Contributor

snide commented Dec 6, 2019

Thanks @i-a-n, no worries about your todo list. We'll jump in and evaluate / clean up. Might not happen till after the holidays depending upon the schedule, but it's a great place to start from a feature perspective. Thanks for the start!

@chandlerprall
Copy link
Contributor

Thanks @i-a-n !!

As markdown processors/renderers are pretty stable around their interface (give text input in, get rendered content out) I think it would make sense to have this component be markdown processor agnostic and allow it to be configured in some fashion.

That would naturally extend this component beyond markdown, and allow other processors/renderers to be used. That continues down the path that maybe the editor bar should be configurable, but I think that we can avoid implementing that as long as we don't do something which would block building it in the future.

@snide
Copy link
Contributor

snide commented Dec 10, 2019

As a heads up, @miukimiu will take a run through this from the design side.

@miukimiu miukimiu self-requested a review December 11, 2019 12:16
@@ -61,6 +61,8 @@
"react-is": "~16.3.0",
"react-virtualized": "^9.21.2",
"resize-observer-polyfill": "^1.5.0",
"showdown": "^1.9.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

The goal will be to not prescribe what library is used and instead only focus on the editor. The rendering library will be configurable.

Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome, thanks for the clarification @chandlerprall!

@miukimiu
Copy link
Contributor

Hi @i-a-n!
Thanks again for giving me access to your branch but I decided to move in a different direction.

I cherry-picked your two commits and created a draft PR that can be found here.

So I'm closing this PR in favor of the new one.

@miukimiu miukimiu closed this Feb 14, 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