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

Make markdown toolbar extensible #33

Merged
merged 2 commits into from
Dec 30, 2021
Merged

Make markdown toolbar extensible #33

merged 2 commits into from
Dec 30, 2021

Conversation

davwheat
Copy link
Member

Replaces #31 because I broke GitHub.

Copy link
Sponsor Member

@askvortsov1 askvortsov1 left a comment

Choose a reason for hiding this comment

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

respectable

@dsevillamartin
Copy link
Member

dsevillamartin commented Oct 25, 2021

Is this easily extendible? I thought using override/extend on exported functions was hard (cause don't you have to go through flarum.core.compat to be able to modify the method?)

Just occurred to me.

@davwheat
Copy link
Member Author

davwheat commented Oct 25, 2021

This should work:

old = markdownToolbarItems;
markdownToolbarItems = function (){
  const items = old();

  items.add(/* ... */);

  return items;
}

@dsevillamartin
Copy link
Member

@davwheat Um, don't think so? Assigning variables from import statements don't change what the import refers to afaik... That'd only work for a global variable or an exported object/class.

@askvortsov1
Copy link
Sponsor Member

This should work:

old = markdownToolbarItems;
markdownToolbarItems = function (){
  const items = old();

  items.add(/* ... */);

  return items;
}

Ah yeah David is right. Until we have an export registry and can actually replace functions in the registry, this won't work.

For some reason I thought this was being added a a method, must have missed this, sorry

Copy link
Sponsor Member

@askvortsov1 askvortsov1 left a comment

Choose a reason for hiding this comment

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

see above

@davwheat
Copy link
Member Author

Hoping that #34 will be merged first, since this PR will need some changes once that is done.

@davwheat
Copy link
Member Author

Ready for review now.

Comment on lines +92 to +96
if (TextEditor.prototype.markdownToolbarItems) {
override(TextEditor.prototype, 'markdownToolbarItems', markdownToolbarItems);
} else {
TextEditor.prototype.markdownToolbarItems = markdownToolbarItems;
}
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Shouldn't we be able to override in either case?

Copy link
Member Author

Choose a reason for hiding this comment

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

When I tried that, it seemed to error at runtime.

Copy link
Member Author

@davwheat davwheat Dec 29, 2021

Choose a reason for hiding this comment

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

image
image

This is because it calls bind on the original function, which doesn't exist.

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Hmm, we might want to change that in core. But makes sense for now.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree

@davwheat davwheat merged commit 1f25327 into master Dec 30, 2021
@davwheat davwheat deleted the dw/extensible-toolbar branch December 30, 2021 21:02
askvortsov1 pushed a commit that referenced this pull request Mar 11, 2022
askvortsov1 pushed a commit that referenced this pull request May 10, 2022
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.

4 participants