Fix first comment in mobile view with french locale #7441
Conversation
Fixes diaspora#7438. The regression has possibly been introduced by diaspora#7207.
|
Ah, I only tested with a post where already were comments ... that explains why I wasn't able to reproduce this ;) |
|
|
||
| Diaspora.Mobile.Comments.increaseReactionCount(this.bottomBar); | ||
| this.toggleReactionsLink = this.bottomBar.find(".show-comments").first(); | ||
| expect(this.toggleReactionsLink.text().trim()).toBe("1 comment"); |
There was a problem hiding this comment.
if the local is set to french, this should be "1 commentaire" I guess?
There was a problem hiding this comment.
The locale is not set to french. I just used the "0 comments" translation from the french locale because that caused the problem.
There was a problem hiding this comment.
I don't get it, if you manually set the text, how does that test anything?
There was a problem hiding this comment.
The code changing the comments count tries to get the number from the bottom bar, increases the number and sets the new text afterwards. In the English locale the text is “0 comments”, so it finds the “0” and is able to parse and increase it. For the French locale you have “Aucun commentaire”, so finding the number and parsing it failed. This PR changes the code to use 0 if it's unable to find a number.
To test the correct behavior I have to set some text that doesn't contain a number and since the issue has been reported for the French locale I used “Aucun commentaire”. Without the change in app/assets/javascripts/mobile/mobile_comments.js the test fails.
There was a problem hiding this comment.
Can't we preload the number (with gon?) instead of parsing the text representation?
There was a problem hiding this comment.
Oh, okay. Yeah, parsing the interface feels a bit hackish.
There was a problem hiding this comment.
We could do lots of things that would include bigger refactorings. ;) This is just a quickfix for the (hackish) implementation.
There was a problem hiding this comment.
Then perhaps it would be nice to create a tracking issue for this hack refactoring, if there isn't one yet.
|
Merged as fc28d6a Thank you 🍪 |
Fixes diaspora#7438. The regression has possibly been introduced by diaspora#7207. closes diaspora#7441
Fixes diaspora#7438. The regression has possibly been introduced by diaspora#7207. closes diaspora#7441
Fixes diaspora#7438. The regression has possibly been introduced by diaspora#7207. closes diaspora#7441
Fixes #7438. The regression has possibly been introduced by #7207.