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

Use github markdown utils in core, support key handlers #2826

Merged
merged 6 commits into from
May 10, 2021

Conversation

askvortsov1
Copy link
Member

@askvortsov1 askvortsov1 commented May 2, 2021

Fixes #2722
Fixes #2746

Background
Right now we have a mix of 3 systems:

  1. Our own BasicEditorDriver for inserting/removing text
  2. mdarea for continuing lists and indent/outdent (in markdown)
  3. Some of Github's markdown-toolbar-element for buttons/commands (in markdown)

Getting them to work together can be a pain. This PR combines (1) and (3), and the accompanying PR to markdown removes (2) for now. I hope to implement / support (2) in core eventually, since continuing lists is quite useful.

Changes proposed in this pull request:
Move the insertText and styleSelectedText utils to core
This simplifies the markdown extension and allows other extensions to use these features.
It also allows undoing stuff like inserting replies/mentions.

In future versions, I'd like to include support for continuing lists and indentation.

Confirmed

  • Frontend changes: tested on a local Flarum installation.
  • Backend changes: tests are green (run composer test).

This simplifies the markdown extension and allows BBCode to use these features.
It also allows undoing stuff like inserting replies/mentions
Copy link
Member

@davwheat davwheat left a comment

Choose a reason for hiding this comment

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

Haven't tested locally yet, just added some notes based on my looking into this yesterday

js/src/common/utils/insertText.ts Outdated Show resolved Hide resolved
js/src/common/utils/insertText.ts Outdated Show resolved Hide resolved
js/src/common/utils/insertText.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@tankerkiller125 tankerkiller125 left a comment

Choose a reason for hiding this comment

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

Testing locally this PR creates an issue with mentions working properly.

2021-05-02.19-35-43.mp4

@askvortsov1
Copy link
Member Author

Testing locally this PR creates an issue with mentions working properly.

2021-05-02.19-35-43.mp4

Nice catch! Fixed

Copy link
Contributor

@tankerkiller125 tankerkiller125 left a comment

Choose a reason for hiding this comment

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

I can confirm that the changes now work correctly. I'm not the best person for reviewing the JS code here so if another review want's the review the actual code that'd be perfect.


this.el.dispatchEvent(new CustomEvent('input', { bubbles: true, cancelable: true }));
items.add('submit', function (e) {
if ((e.metaKey || e.ctrlKey) && e.key === 'Enter') {
Copy link
Member

Choose a reason for hiding this comment

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

Is Win + Enter causing a post to submit intentional, as it seems like it will do.

https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/metaKey

Copy link
Member

@davwheat davwheat May 6, 2021

Choose a reason for hiding this comment

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

In Markdown, we check against the user agent to decide between Cmd and Ctrl.

Maybe we could add the getOS helper I had in a PR a while back to make this info part of core and available to extensions instead of duplicating logic.

Something like app.operatingSystem or window.__operatingSystem?

Copy link
Member Author

@askvortsov1 askvortsov1 May 8, 2021

Choose a reason for hiding this comment

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

In the long term, I'd to introduce some form of "normalize keys" util like exists in prosemirror, which turns Meta-KEY into the appropriate cmd vs ctrl. I haven't found a library yet.

For now, I'm not sure this PR is the best place to change existing behavior?

Copy link
Contributor

Choose a reason for hiding this comment

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

If this is the current action of MDArea/Prosmirror then I say we keep it this way for now and modify it later once we normalize things.

js/src/common/utils/BasicEditorDriver.ts Outdated Show resolved Hide resolved
@askvortsov1 askvortsov1 requested a review from davwheat May 8, 2021 01:39
@askvortsov1 askvortsov1 merged commit dd8323e into master May 10, 2021
@askvortsov1 askvortsov1 deleted the as/editor-consolidation branch May 10, 2021 21:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants