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

Add reopenWidget method #4657

Merged
merged 4 commits into from Feb 1, 2017
Merged

Conversation

gdpelican
Copy link
Contributor

@gdpelican gdpelican commented Jan 16, 2017

This is a little bit of mad science that I needed for Babble; I figured I'd offer it up; you may or may not want to support it in core.

This allows us to add or override existing methods / properties on widgets, similar to Ember's reopenClass method. I found myself needing it to overwrite the url() method in notification-item, because I want to point chat notifications to the full page chat. I call it like this:

import { reopenWidget } from 'discourse/widgets/widget'

reopenWidget('notification-item', {
  url() {
    console.log("I am a custom function!")
  }
})

Supporting this._super() as well would be ideal here, but Ember's got a whole crazy scheme for supporting that, which I don't really want to reimplement (I was able to get the super method in other ways)

@mention-bot
Copy link

Thanks @gdpelican for your pull request 👍. By analyzing the blame information on this pull request, I identified @eviltrout and @tgxworld to be potential reviewers.

@discoursebot
Copy link

You've signed the CLA, gdpelican. Thank you! This pull request is ready for review.

@eviltrout
Copy link
Contributor

This seems good to me, but I do think it should be a PluginAPI and exposed that way, rather than plugins just requiring the method and reopening themselves.

@gdpelican
Copy link
Contributor Author

Okay, I've updated this to be called this way:

    withPluginApi('0.1', api => {
      api.reopenWidget('notification-item', {
        url() {
          console.log('some new implementation')
        }
      })
    })

* See `reopenWidget` in `discourse/widgets/widget` from more ifo.
**/

reopenWidget(name, args) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You'll also need to bump the API version, otherwise people don't have a version to tie this new function to.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, don't quite follow that yet. Is this the current version here?

function getPluginApi(version) {
  version = parseFloat(version);
  if (version <= 0.6) {

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes! If you just update that to 0.7 then anyone who uses 0.7 will be guaranteed to have your new function.

@gdpelican
Copy link
Contributor Author

Cool, I've popped that up to 0.7 now. Let me know if there's anything else? @eviltrout

@eviltrout eviltrout merged commit cef8a0a into discourse:master Feb 1, 2017
@eviltrout
Copy link
Contributor

Thanks, it looks great!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
5 participants