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

following category on the category page #2312

Closed
wants to merge 12 commits into from

Conversation

Catrin
Copy link
Contributor

@Catrin Catrin commented May 5, 2014

I added a dropdown button on the category page where a logged in user can change the notification level of the category.
latest_meta_topics_-_discourse

@discoursebot
Copy link

Thanks for contributing this pull request! Could you please sign our CLA so we can review it? http://www.discourse.org/cla

@SamSaffron
Copy link
Member

very interesting, we always wanted something like this. what happens by default here? looks like the drop down only has overrides?

@Catrin
Copy link
Contributor Author

Catrin commented May 5, 2014

Just like the topic-notification-button the default ist "regular", but there is no possibility to remove the notification. This can only be done in the preferences.

@SamSaffron
Copy link
Member

I think we would need all 4 options, but I am very concerned about real estate, how can we fit this here? At most we have space for a glyph imo.

image

@Catrin
Copy link
Contributor Author

Catrin commented May 5, 2014

ok good point. I have forgotten the edit-button. Then I'll add the fourth option and remove the text from the button and just leave the glyph. ok?

@Catrin
Copy link
Contributor Author

Catrin commented May 5, 2014

now (after the travis hopefully did not find any bugs or failing tests ;) ) there are all 4 options and only the glyph left in the button.

@SamSaffron
Copy link
Member

Can you update the screen shot? @coding-horror any thoughts about this?

@Catrin
Copy link
Contributor Author

Catrin commented May 6, 2014

Good Morning :) here is the new screenshot.
latest_uncategorized_topics_-_discourse

@coding-horror
Copy link
Contributor

I think it looks fantastic!

@SamSaffron
Copy link
Member

One last thing here, we really need the glyphs next to the text in the drop down (like we have now in the standard dropdown for topic notification levels)

@Catrin
Copy link
Contributor Author

Catrin commented May 6, 2014

ok, I'll add them.

@Catrin
Copy link
Contributor Author

Catrin commented May 6, 2014

This is what it looks like after the latest commit: latest_uncategorized_topics_-_discourse

@coding-horror
Copy link
Contributor

looks good! Regular is simply text no glyph, though I agree if we are using a glyph at the top we have to choose something to put there.

@SamSaffron
Copy link
Member

we are so close :) thanks heaps.

I meant consistency with

image

So you probably need to add a glyph over there for regular (I am fine with the open circle) and then use same glyphs in both spots .

@Catrin
Copy link
Contributor Author

Catrin commented May 7, 2014

no problem. I change the glyphs.
Should I add the circle glyph to the list next to regular or just show it in the button?
Meanwhile I try to unterstand why the build does not pass :-/

@SamSaffron
Copy link
Member

Lets try with the glyph omitted for now.

These build failures do not look like they are your fault.

Can you merge in latest, it should fix it.

@Catrin
Copy link
Contributor Author

Catrin commented May 7, 2014

next try :)
screenshot_07_05_14_11_50

@SamSaffron
Copy link
Member

much better, but one super last thing which is also awesome practice for you :)

we essentially have 2 areas of code that are doing exactly the same thing

Discourse.CategoryNotificationsButton and Discourse.NotificationsButton

Instead we should centralize this and only use Discourse.NotificationsButton passing it in all the differences (which is only the i18n key)

That way we don't need to carry around dupe code.

Once you do that can you rebase ?

@Catrin
Copy link
Contributor Author

Catrin commented May 8, 2014

ohhh, I was so glad that it "somehow" worked ;) Not that easy to get into discourse.
But yes, I also considered moving the dublicated code up. I'll digg into that tonight...

@Catrin
Copy link
Contributor Author

Catrin commented May 8, 2014

On 07 May 2014, at 22:14, Dung Quang notifications@github.com wrote:

is this something related to tracked and watched categories in user preferences ?

Yes, it is.
What I am trying to implement is, that you can change the notification level directly on the category page in addition to the possibility in the user preferences.

@coding-horror
Copy link
Contributor

This is really great. When can we merge this?

@riking
Copy link
Contributor

riking commented May 15, 2014

@Catrin Ping! What's your status? This looks great.

@Catrin
Copy link
Contributor Author

Catrin commented May 15, 2014

Working on it. I do not want to put everything into the Discourse.NotificationsButton. So my new plan is: Having Discourse.NotificationsButton as a super class and TopicsNotificationsButton and CategoryNotificationsButton below.

@coding-horror
Copy link
Contributor

Looking forward to it 👀 let us know how we can help

@coding-horror
Copy link
Contributor

Again, let us know how we can help! Do you think this can be done by the end of the week?

@Catrin
Copy link
Contributor Author

Catrin commented May 23, 2014

I am so sorry. I could not finish it. Too busy working on a different project (and my kids...).

@coding-horror
Copy link
Contributor

OK no problem thanks for letting us know! We'll pick it up from here, I'd really like to get this in.

@nlalonde
Copy link
Member

I'll finish this.

@nlalonde nlalonde self-assigned this May 27, 2014
@coding-horror
Copy link
Contributor

Thanks @nlalonde looking forward to it. And thank you @Catrin for getting us so far on what I think is a GREAT addition to Discourse. 💝

@nlalonde
Copy link
Member

I merged this manually and implemented your suggestion to "have Discourse.NotificationsButton as a super class and TopicsNotificationsButton and CategoryNotificationsButton below."

This feature is very handy!

@nlalonde nlalonde closed this May 29, 2014
@Catrin
Copy link
Contributor Author

Catrin commented May 30, 2014

Hey Neil,

great and great code :) Thank so much you for taking over.
(Next time I’ll finish my stuff :))

On 30 May 2014, at 00:28, Neil Lalonde notifications@github.com wrote:

I merged this manually and implemented your suggestion to "have Discourse.NotificationsButton as a super class and TopicsNotificationsButton and CategoryNotificationsButton below."

This feature is very handy!


Reply to this email directly or view it on GitHub.

@coding-horror
Copy link
Contributor

@Catrin actually @nlalonde said it was kind of a pain to refactor the code, so it ultimately was asking a lot. Thank you for getting it started and pushing it along so far. Don't hesitate to propose other features on meta.discourse, even if you don't have time to implement them..

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