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

FEATURE: Split Reply-button into 'Reply Post' and 'Reply Topic' #4600

Merged
merged 2 commits into from Jan 26, 2017

Conversation

jsilvanus
Copy link
Contributor

@jsilvanus jsilvanus commented Dec 13, 2016

This feature should enable giving a different label for 'Reply Topic' and 'Reply Post'. I hope I didn't forget anything.

I also considered moving the button label to controls.reply, but that was already taken for the tooltip for said button, whereas label for the button is taken from topic.reply.title, so I just elaborated on the current state.

@discoursebot
Copy link

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

@jsilvanus
Copy link
Contributor Author

I must admit that I did not have a dev discourse running, so I didn't do a test on this... Will try to do it later!

message:
title: 'Reply'
# help i.e. tooltop is at controls.reply
thread:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm. Now that I think of it, I think that "topic" instead of "thread" would follow the naming convention...

Copy link
Member

Choose a reason for hiding this comment

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

I agree. We never use "thread" in Discourse.

message:
title: 'Reply'
# help i.e. tooltop is at controls.reply
thread:
Copy link
Member

Choose a reason for hiding this comment

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

I agree. We never use "thread" in Discourse.

@jsilvanus
Copy link
Contributor Author

@ZogStriP, changed this to follow naming convention: now post & topic instead of message & thread. :)

@jsilvanus
Copy link
Contributor Author

jsilvanus commented Dec 13, 2016

Few thoughts:

  1. I considered long and hard, whether the tooltip should be moved from controls.reply to post.reply.post.help, but decided not to go for this.

  2. I made the changes into all the client.*.yml. It's in its own branch locale in my fork:
    c691759

@jsilvanus jsilvanus changed the title FEATURE: Split Reply-button into two FEATURE: Split Reply-button into 'Reply Post' and 'Reply Thread' Dec 13, 2016
@jsilvanus jsilvanus changed the title FEATURE: Split Reply-button into 'Reply Post' and 'Reply Thread' FEATURE: Split Reply-button into 'Reply Post' and 'Reply Topic' Dec 13, 2016
@ZogStriP
Copy link
Member

I made the changes into all the client.*.yml. It's in its own branch locale in my fork: c691759

This wasn't required since all the translations are handled via transifex only.

@ZogStriP
Copy link
Member

I will merge after the next version bump, that way, translators will have time to fix the strings.

@ZogStriP ZogStriP merged commit f46f8ff into discourse:master Jan 26, 2017
@ZogStriP
Copy link
Member

Sorry for the delay @jsilvanus. Thanks for the PR 👍

@coding-horror
Copy link
Contributor

This PR caused a lot of damage -- broken translations in every language, including English. We are in the process of reverting it.

Do not submit this again; it will not be accepted.

@jsilvanus
Copy link
Contributor Author

I thought I made clear I had another PR that has the changes for ymls... I was told it wasn't needed as the ymls were handled differently. I'm sorry for the trouble.

@coding-horror
Copy link
Contributor

We are still dealing with the fallout from this disastrous change:

https://meta.discourse.org/t/turkish-translation-needs-some-love/57101

coding-horror added a commit that referenced this pull request Feb 8, 2017
I hate that PR with the fire of a thousand suns
@jsilvanus
Copy link
Contributor Author

jsilvanus commented Feb 27, 2017

Can I ask what was wrong with the PR? Was the initial idea of splitting the two buttons (which have different functionality but have the same string attached to them) wrong, or was my code somehow disastrous? I had all the translations in another branch, but I was told not to include them.

@ZogStriP
Copy link
Member

ZogStriP commented Feb 27, 2017

It was because this changed the translation of a very important feature and our client-side translation code wasn't properly falling back to English when the translation was unavailable. Thus basically breaking non-English Discourses.

As for other translations, they would have been overwritten on our next pull from transifex anyway. So they would not have been useful.

Ultimately, I'm the one to blame here since I merged that PR without considering all its consequences.
Don't worry too much about that 😉

@jsilvanus
Copy link
Contributor Author

Thank you, this clarifies a bit more what happened. I was in a week long training - and thus unable to participate in the discussion - while I suddenly got a notification on my mobile that my PR had caused enormous trouble and I was all "OMG WHAT DID I DO"...

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