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

Implement internationalization PapercupsIntl() + some fixes #77

Merged
merged 17 commits into from
Dec 9, 2021

Conversation

CharlesMangwa
Copy link
Contributor

@CharlesMangwa CharlesMangwa commented Dec 9, 2021

Hi @aguilaair!

This PR implements a couple of things:

  1. The new PapercupsIntl() class that brings translations for all strings I was able to find in the source code. One can now use it as such:
PaperCupsWidget(
  props: Props(
    accountId: "xxxxxxxx-xxxxxxx-xxxx-xxxxxx",
    translations: PapercupsIntl(
      title: "Welcome 👋"
      subtitle: "How can we help you today?",
      greeting: "We'll make sure to reply in an hour or less",
      // ...
    ),
  ),
)

⚠️ BREAKING CHANGE

This is a breaking change as I've removed the duplicated properties from both Props and PapercupsWidget. My rationale is that this could be valid enough for a major release if coupled with the Availability feature for instance. Let me know If you'd rather have it in a minor release so that I add deprecation notices a little bit everywhere.

  1. A fix for a bug I found when running apps in profile or release mode: event.event.toString() only returns "PhoenixChannelEvent" in those modes. So this case was never true

    if (event.event.toString().contains("shout") ||
    event.event.toString().contains("message:created")) {

    which means that whenever a message was sent from the dashboard to an app, it wouldn't show up in real-time.

  2. Replacing the use of Papercups fullName for displayName as that seems more appropriate in this context.

  3. A couple of minor typos corrections here and there.

@aguilaair
Copy link
Collaborator

This looks awesome 😎

TYSM, I'll review it ASAP

Copy link
Collaborator

@aguilaair aguilaair left a comment

Choose a reason for hiding this comment

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

LGTM!

@@ -30,7 +30,8 @@ PhoenixChannel? joinConversationAndListen({
conversation = null;
} else {
if (event.event.toString().contains("shout") ||
event.event.toString().contains("message:created")) {
event.event.toString().contains("message:created") ||
event.payload!["type"] == "reply") {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This was a great catch!

@aguilaair
Copy link
Collaborator

I'll merge this now. However, I will not make a release quite yet because there are some kinks in the file upload I want to iron out, specifically want the upload to happen inside of lib/widgets/chat/attachment.dart instead of the sendMessage function, this should help concurrency and we can also show more progress details instead of having a snackbar do that.

@aguilaair
Copy link
Collaborator

closes #67

@aguilaair aguilaair linked an issue Dec 9, 2021 that may be closed by this pull request
@aguilaair aguilaair added bug Something isn't working enhancement New feature or request labels Dec 9, 2021
@aguilaair aguilaair merged commit b33b940 into papercups-io:main Dec 9, 2021
@aguilaair
Copy link
Collaborator

Regarding a major version release, I agree that this being a breaking PR it should be released as a major version in order to ensure minimal issues. We can (maybe?) pair it with attachments, although availability should be able to be implemented now too, I'll eventually get to it but right now I have no time to work on it. Hopefully will get a chance in the next month or so, or maybe you wanna take a stab at it? Let me know and I'll assign it, @CharlesMangwa.

@CharlesMangwa
Copy link
Contributor Author

Thanks for the quick merge @aguilaair! And yes, I can look into Availability. I'd just need a little bit of guidance as I'm not really familiar with the API and even after looking in the Presence docs, I didn't quite get what I'm supposed to get 😅 But I guess we can continue this convo in #65.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fully internationalize the widget
2 participants