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

library: Add Email Entry #330

Merged
merged 14 commits into from Jun 17, 2023
Merged

library: Add Email Entry #330

merged 14 commits into from Jun 17, 2023

Conversation

SoNiC-HeRE
Copy link
Contributor

Closes #329

@SoNiC-HeRE
Copy link
Contributor Author

@andyholmes Can you please provide an early review for the email demo?
As of last time it seemed to work for everyone else, so just wanna make sure if it's a permission error or something else

Copy link
Contributor

@sonnyp sonnyp left a comment

Choose a reason for hiding this comment

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

works for me

the issue might be that you don't have an email application

@sonnyp
Copy link
Contributor

sonnyp commented Jun 14, 2023

Please make sure to use Gio._promisify

@SoNiC-HeRE
Copy link
Contributor Author

SoNiC-HeRE commented Jun 14, 2023

Please make sure to use Gio._promisify

Yes , i was just making sure if it was a problem on my side.

Idk if it's relevant or not but shouldn't it be triggered if there's no email app on the user?

For example: if user hasn't installed any email app, browser should be accessed with an email template

@SoNiC-HeRE
Copy link
Contributor Author

This worked fine for me when i installed the app :)

Copy link
Contributor

@sonnyp sonnyp left a comment

Choose a reason for hiding this comment

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

It would be nice to add a text input inviting the user to enter their email address and use it for the addresses arg.

This way, when the user can test by sending the email to themselves directly.

wdyt?

using Adw 1;

Adw.StatusPage {
title: "Email";
Copy link
Contributor

Choose a reason for hiding this comment

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

all user visible strings should be wrapper in _()

please try to remember

Suggested change
title: "Email";
title: _("Email");

label: _("Send Email");
margin-bottom: 42;
halign: center;
styles ["suggested-action"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
styles ["suggested-action"]
styles ["suggested-action", "pill"]

for buttons outside of toolbars

["sonicworks05@gmail.com"],
["test@gmail.com"],
null,
"Demo Message",
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add a comment next to each line with the name of the argument so it's clear what is what?

For example here

Suggested change
"Demo Message",
"Demo Message", // subject

Comment on lines 18 to 19
["sonicworks05@gmail.com"],
["test@gmail.com"],
Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't use your personal data in examples.

Suggested change
["sonicworks05@gmail.com"],
["test@gmail.com"],
["example@example.com"], // addresses
null, // cc

}
}

button.connect("clicked", sendEmail);
Copy link
Contributor

Choose a reason for hiding this comment

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

I have updated our practice a bit here see #339

Please remove the try catch from sendEmail to make the function simpler and do the following instead

Suggested change
button.connect("clicked", sendEmail);
button.connect("clicked", () => {
onClicked().catch(logError);
));

Comment on lines 18 to 19
["sonicworks05@gmail.com"],
["test@gmail.com"],
Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't use your personal data in examples.

Suggested change
["sonicworks05@gmail.com"],
["test@gmail.com"],
["example@example.com"], // addresses
null, // cc

["test@gmail.com"],
null,
"Demo Message",
"Demo Content",
Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest "Email from Workbench" as subject and "Hello World!" as body

{
"name": "Email",
"category": "platform",
"description": "Compose an email message",
Copy link
Contributor

@sonnyp sonnyp Jun 15, 2023

Choose a reason for hiding this comment

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

remember to write description from the perspective of the developer

Suggested change
"description": "Compose an email message",
"description": "Trigger an email",


async function sendEmail() {
try {
const result = await portal.compose_email(
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const result = await portal.compose_email(
const success = await portal.compose_email(

Comment on lines 28 to 30
if (result) {
console.log("Email app opened");
}
Copy link
Contributor

Choose a reason for hiding this comment

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

It could be a web app or the app could already be opened. It's best not to be overly specific if we don't know for sure what will happen.

Also we know from your experience what a case for failure can be so maybe we can help the user figure it out.

Suggested change
if (result) {
console.log("Email app opened");
}
if (success) {
console.log("Success");
} else {
console.log("Failure, verify that you have a register email application.");
}


Adw.StatusPage {
title: "Email";
description: _("Compose an email");
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
description: _("Compose an email");
description: _("Trigger an email");

@SoNiC-HeRE
Copy link
Contributor Author

It would be nice to add a text input inviting the user to enter their email address and use it for the addresses arg.

This way, when the user can test by sending the email to themselves directly.

wdyt?

Sure, it's a good idea

@SoNiC-HeRE
Copy link
Contributor Author

Made the following changes

  • Used a Gtk.Entry to take email from user
  • Commented various properties of portal.compose_email method
  • Implemented an email validator
  • Refined Code
  • Removed async handlers

@SoNiC-HeRE SoNiC-HeRE requested a review from sonnyp June 16, 2023 16:52
SoNiC-HeRE and others added 10 commits June 17, 2023 19:12
* Adds Adw.AboutWindow Demo
* Updated Link
* Updated Code
* Updated Icon
* Added Acknowledgement Section
* library: Add AdwAnimation entry
* Animation: Fix CallbackAnimationTarget and add AnimationTarget example
* Animation: Add links
* Animation: Changes from review
* Animation: Update main.css
* Added TextView Demo
* Updated JS Code
* Updated Code
* Formatting Changes
* libary: Add SpinButton entry
* Spin Button: Add link to tutorial
* Spin Button: Changes from review
* Spin Button: Translated strings
* Spin Button: Add comments for clarity
* library: Add AdwBanner entry
* Banner: Translate strings
Copy link
Contributor

@sonnyp sonnyp left a comment

Choose a reason for hiding this comment

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

Well done.

I made minor changes a3966fb

Make sure to use the name of arguments as defined by the API documentation next time.

@sonnyp sonnyp changed the title library: Adds Email Entry library: Add Email Entry Jun 17, 2023
@sonnyp sonnyp merged commit b91b013 into main Jun 17, 2023
@sonnyp sonnyp deleted the sonic/email branch June 17, 2023 17:19
sonnyp pushed a commit that referenced this pull request Jun 20, 2023
sonnyp pushed a commit to SoNiC-HeRE/Workbench that referenced this pull request Aug 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adds Email Demo
5 participants