Remove MarkdownEditor from @primer/react

[iansan5653]

In order to more easily maintain and iterate on the MarkdownEditor component, we'd like to pull it back into GitHub's internal codebase. As this component never left drafts, we should be able to do this without a breaking change.

The tricky part here is going to be figuring out what to do with the internal dependencies of the component. There are two options, and the correct choice may be different for each dependency:

1. Remove it from @primer/react and pull it into the internal codebase along with the MarkdownEditor
2. Expose it publicly from @primer/react so it can be used in the internal codebase and in other places

I've taken a stab at going through the dependency tree and trying to figure out what to do with each thing. Ignoring things that are internal to the component or already public:

• Internal hooks:
  • useResizeObserver: Let's just expose this publicly by exporting it from hooks/index.ts. I think this is useful and low-risk. Already exposed publicly.
  • useSlots: I think we've previously avoided making this public because it was very unstable, but we seem to have settled on a pattern that works. This would be really useful to have access to in other codebases for making reusable components - let's expose this in hooks/index.ts as well. from drafts.
• Internal components:
  • VisuallyHidden: This component is private in @primer/react, but is duplicated already in the Memex codebase. I think we should go ahead and just make it public so we can remove the duplication. Maybe we can expose it from @primer/react/drafts? We will duplicate this into the internal codebase.
  • InputLabel (required by MarkdownEditor.Label): This is private in Primer and used in the editor to reuse the styling. I don't think it makes sense to expose this so there's not really a good path here except to just duplicate the styles.
• Draft components:
  • MarkdownViewer: Migrate this along with MarkdownEditor. This is easy - it has no subdependencies that aren't public.
  • InlineAutocomplete: Migrate
    • This hooks in to FormControl, so we need to make that API public as well
• Utils:
  • character-coordinates: Migrate, moving internal to InlineAutocomplete
• Publicly exposed draft hooks: these are public and used fairly often in other codebases. I think we should just leave them behind and keep them publicly exposed in @primer/react/drafts/hooks: We will migrate these as well:
  • useCombobox
  • useDynamicTextareaHeight
  • useIgnoreKeyboardActionsWhileComposing
  • useSafeAsyncCallback
  • useSyntheticChange
  • useUnifiedFileSelect

When done we'll also be able to remove the following dependencies:

• fzy.js
• @github/markdown-toolbar-element
• @github/paste-markdown

I propose we do this in three stages:

1. Expose the internal code that we want to continue sharing with the editor (useResizeObserver, useSlots, and VisuallyHidden), and cut a release of @primer/react
2. Copy the to-be-migrated pieces (MarkdownEditor, MarkdownViewer, InlineAutocomplete and dependencies) from this repo into the internal GitHub repo
3. Delete the migrated code from @primer/react

[siddharthkp]

This is an excellent writeup! I appreciate the dry-run before we pull the components out ❤️

Internal hooks:

• useResizeObserver: Let's just expose this publicly by exporting it from hooks/index.ts. I think this is useful and low-risk.

This is already exported from top level @primer/react index :)

• useSlots: I think we've previously avoided making this public because it was very unstable, but we seem to have settled on a pattern that works. This would be really useful to have access to in other codebases for making reusable components - let's expose this in hooks/index.ts as well.

I am a bit afraid of exporting this, I think you already know why. But, I agree with you, we do have a stable pattern for now. Let's ship it from drafts to begin with and track it's usage?

Internal components:

• VisuallyHidden: This component is private in @primer/react, but is duplicated already in the Memex codebase. I think we should go ahead and just make it public so we can remove the duplication. Maybe we can expose it from @primer/react/drafts?

It is a slightly controversial component because it's meant to be used very rarely as it's easy to create a11y bugs with it. (we even keep it in src/internal and src/).

For now, I think we should create a copy in MarkdownEditor. Let's revisit this when we're further along in accessibility remediations and have more confidence if this is a pattern we want to avoid OR document and promote safe usage.

• InputLabel (required by MarkdownEditor.Label): This is private in Primer and used in the editor to reuse the styling. I don't think it makes sense to expose this so there's not really a good path here except to just duplicate the styles.

Agreed. We should duplicate the styles.

Draft components:

• MarkdownViewer: Migrate this along with MarkdownEditor. This is easy - it has no subdependencies that aren't public.
• InlineAutocomplete: Migrate

Perfect and perfect.

Utils:

• character-coordinates: Migrate, moving internal to InlineAutocomplete

Perfect.

• Publicly exposed draft hooks: these are public and used fairly often in other codebases. I think we should just leave them behind and keep them publicly exposed in @primer/react/drafts/hooks:
• useCombobox
• useDynamicTextareaHeight
• useIgnoreKeyboardActionsWhileComposing
• useSafeAsyncCallback
• useSyntheticChange

I have a slightly different outlook here.

If the hooks are used only in the components that we are moving to ui/packages (MarkdownEditor, InlineAutocomplete), we should move the hooks too to ui/packages.

For example, useCombobox is only used in InlineAutocomplete (along with the dependency@github/combobox-nav)

I see the wider application of something like useDynamicTextareaHeight but if they are not used in primer/react components today, we should simply move these hooks in ui/packages.

Again, this might change in the future with primer, and we can upstream a hook back into primer/react when it happens.

[iansan5653]

Cool, I'm good with all those decisions. I'll open a PR to expose useSlots from drafts, and I'll take a look a the hooks to see which ones (if any) are used in other components.

[cheshire137]

In order to more easily maintain and iterate on the MarkdownEditor component, we'd like to pull it back into GitHub's internal codebase. As this component never left drafts

Is the idea that we'd iterate more on it internally till we were satisfied with it, then move it back to @primer/react someday when it's stood the test of all our internal use cases?

[cheshire137]

Publicly exposed draft hooks: these are public and used fairly often in other codebases. I think we should just leave them behind and keep them publicly exposed

"Leave them behind"... Are you saying you think we should duplicate these draft hooks in the internal GitHub repo? Why duplicate versus use the publicly exposed one in Primer?

[iansan5653]

Is the idea that we'd iterate more on it internally till we were satisfied with it, then move it back to @primer/react someday when it's stood the test of all our internal use cases?

Possibly, but I don't think this is a declared goal. The context here is that we originally placed this here because we had no internal way to share code. Now that we do, it's just added friction having it in Primer. Particularly when we consider ownership -- technically this code is owned by the Primer team, but in reality I've been the only one maintaining it. If we move it back into the monolith, then the new shared components team can take it on. Having it in the monolith fits better with our vision for the purpose and maintenance plan of ui/packages.

---

Publicly exposed draft hooks: these are public and used fairly often in other codebases. I think we should just leave them behind and keep them publicly exposed

"Leave them behind"... Are you saying you think we should duplicate these draft hooks in the internal GitHub repo? Why duplicate versus use the publicly exposed one in Primer?

Here I was proposing that we don't copy them at all and just leave them solely in Primer. But I think we'll go with @siddharthkp's recommendation instead:

If the hooks are used only in the components that we are moving to ui/packages (MarkdownEditor, InlineAutocomplete), we should move the hooks too to ui/packages.

So ultimately they will only exist in github/github.

[cheshire137]

Good deal, thanks for the explanation!

we had no internal way to share code

The irony of saying this while being employees of GitHub kind of kills me. 😅 😆

[henrijoss]

Will the MarkdownEditor component be available publicly in Primer again in the future?

[iansan5653]

Will the MarkdownEditor component be available publicly in Primer again in the future?

Maybe! There's some early work in progress but we aren't ready to commit to anything at this point.

[github-actions]

Hi! This issue has been marked as stale because it has been open with no activity for 180 days. You can comment on the issue or remove the stale label to keep it open. If you do nothing, this issue will be closed in 7 days.