Added Support for Custom Message #43
Conversation
There was a problem hiding this comment.
Thanks! Some comments / changes please:
- to match the stye of the rest of the code, please have a space after commas separating things, e.g.
versionCodeUpdateAlertType,messageshould beversionCodeUpdateAlertType, message - please change the documentation to be something more like If for example, you'd like to display a custom message like: "Please update as this version will stop working". The Hindi example seems less general, because how would you know if a specific user requires Hindi? I.e. If you know that ahead of time, why not add the localisation to the library.
- I have not run the library so I may be wrong, but it seems like it will always have the default message of
"message"instead of what used to be there. - Related to above: please create a new test in the test suite that clearly shows how if a message is supplied it is used, and if a message is not in the returned JSON, then the previous behaviour happens as expected (i.e. the default message is used).
Thanks!
|
Sure, I will make the changes. The default message is still what used to be, i.e If no message node or an empty node Custom message is shown only when a non-empty message is sent from the API |
Glad to hear it. Figured it probably was. I didn't try it and obviously didn't read the code closely enough. Thanks for that reassurance. It will be good to have it in the tests regardless. Thanks! |
|
@ankitguptag18 Getting there 🎉 👍 It just needs some tests to demonstrate that the code behaves appropriately with and without the "message" in the fetched JSON. |
|
Hi @mikemee Do I need to do something else? |
|
@ankitguptag18 - oops, sorry! Dropped off my list. I'll try to get to it this week. |
|
Hi, |
|
HI @mikemee Do let me know If I missed anything. |
|
I'm so sorry. Just swamped! |
|
Hi @mikemee Just wondering if I need to do anything else. Please do let me know if I have missed out anything. |
|
ankitguptag18 - Oh dear, another month. I'm sorry! The problem is that for this PR I actually need to download and try it out... and I'm still swamped. Things should get significantly better next week. It's been a busy couple of months. |
|
@mikemee just wondering if there is anything else I need to do. Just to let you know we have taken the code to production months ago and is working without any issues. :) |
|
@mikemee sorry to bother you, It's been almost a year for the PR. Please do let me know if there is anything else I need to do. |
|
@ankitguptag18 OMG. A year! I'm so sorry. I've been living the world of hell that is reserved for iOS developers, dealing with test breakages from Xcode updates and new releases and general pain. So my Android apps have just been running along nicely without any problems – and hence no need to update. I won't promise any dates, as clearly that's meaningless based on past comments, but I am happy to merge this and will do so as soon as I get back to do some new releases of my own (which I hope to do before the end of the year ... but will see.) Apologies again. I'm glad you're at least able to get it into production and move forwards. |
|
@mikemee any update on the merger. The reason for me asking you, again and again, is I would like to remove the Jar file from my project and would like to import the library via gradle config. So if you could merge the code(After verification) and create a tag, that would be really helpful. |
|
Waiting for this feature .please merge ASAP . |
|
Sorry for the delay everyone. I'm getting back to Android coding after a bit of a detour with iOS 🙄 Per the JitPack documentation, anyone can use this PR by using a different URL in your Gradle build file. E.g. instead of a dependency of:
try:
|
You can send a custom message from the server side. e.g if the user language is currently not supported by the library, you can send the message from the server.