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

adding the category name to the title #1

Conversation

deargle
Copy link

@deargle deargle commented Aug 25, 2016

helpful because otherwise there's no indication of which category it is posted to. Feels like an email notification this way.

Am I even sending this PR to the right place? :-\

helpful because otherwise there's no indication of which category it is posted to. Feels like an email notification this way.
@deargle deargle changed the title adding the category slug to the title adding the category name to the title Aug 25, 2016
@richardxia
Copy link
Owner

I'm happy to review this this, but I originally created this fork because bernd#7 has yet to be accepted into the official repo. https://github.com/bernd/discourse-slack-plugin is the original repo, and my branch here is inspired by bernd#7 with some of my own changes. You may want to bump that PR to see if the original author is willing to accept the change.

As for this PR, it's possible for the category variable to be undefined, since it is only assigned to from within the if SiteSetting.allow_category_slack_channel. You probably want to initialize category to nil outside of the if block, and then in the line where you've changed the :title, add a conditional to only prepend the category if it isn't nil.

@deargle
Copy link
Author

deargle commented Aug 25, 2016

As for this PR, it's possible for the category variable to be undefined, since it is only assigned to from within the if SiteSetting.allow_category_slack_channel. You probably want to initialize category to nil outside of the if block, and then in the line where you've changed the :title, add a conditional to only prepend the category if it isn't nil.

Oh duh, of course. Sorry for the sloppy request.

I'm happy to review this this, but I originally created this fork because bernd#7 has yet to be accepted into the official repo. https://github.com/bernd/discourse-slack-plugin is the original repo, and my branch here is inspired by bernd#7 with some of my own changes. You may want to bump that PR to see if the original author is willing to accept the change.

Good idea.

@deargle deargle closed this Aug 25, 2016
@deargle
Copy link
Author

deargle commented Aug 25, 2016

It should probably be more complex than this anyways, because if there's only one channel per category, the category name might be undesirable. My use case is that I'm only sending a subset of my categories to one channel (for example I'm excluding Staff), so it's helpful for me to both be able to per-category things and leave the default category blank, and to also show the category name in the title. Maybe if someone comes along they'll see this closed PR and it'll help them though. I'll drop a comment in on the original PR too.

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.

2 participants