Skip to content

smtp update#384

Merged
christyjacob4 merged 45 commits into1.4.xfrom
feat-custom-smtp
Aug 9, 2023
Merged

smtp update#384
christyjacob4 merged 45 commits into1.4.xfrom
feat-custom-smtp

Conversation

@lohanidamodar
Copy link
Member

@lohanidamodar lohanidamodar commented Apr 10, 2023

What does this PR do?

(Provide a description of what this PR does.)

Test Plan

(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your changes work.)

Related PRs and Issues

Have you read the Contributing Guidelines on issues?

(Write your answer here.)

@vercel
Copy link

vercel bot commented Apr 10, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
console ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 9, 2023 9:45am
console-1-3-preview ❌ Failed (Inspect) Aug 9, 2023 9:45am
console-cloud ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 9, 2023 9:45am
console-next ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 9, 2023 9:45am

export let dismissible = false;
export let type: 'info' | 'success' | 'warning' | 'error' = 'info';
export let buttons: Buttons[] = [];
export let isAction = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this? I don't see where it's used. Also, it might not be clear to have two ways to control whether the action buttons show.

Copy link
Contributor

Choose a reason for hiding this comment

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

We needed it in some initial design but now is not needed for this specific branch :)

Comment on lines +23 to +26
export let fullWidth = false;
export let autofocus = false;
export let interactiveOutput = false;
// Input value
export let stretch = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

why were fullWidth and stretch added? I tried to compare the existing params with the new, but they look the same:

image

Copy link
Contributor

Choose a reason for hiding this comment

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

fullWidth gives it the inline-size: 100% while stretch is flex:1

Copy link
Contributor

Choose a reason for hiding this comment

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

@ArmanNik it's still a little confusing because, as someone who's just using the InputSelectSearch component, I'm not sure when/why I would use one or the other.

Comment on lines +28 to +31
addNotification({
type: 'error',
message: error.message
});
Copy link
Contributor

Choose a reason for hiding this comment

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

In loadSmsTemplate(), there's a try/catch that can catch an error if sdk.forConsole.projects.getSmsTemplate() fails, but that means loadSmsTemplate() would return null/undefined in those cases. That would be problematic, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

mmmmm in what way 🤔 we'd just show a notification right?

Copy link
Contributor

Choose a reason for hiding this comment

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

@ArmanNik, wouldn't we add 2 error notifications then:

  1. when sdk.forConsole.projects.getSmsTemplate() in loadSmsTemplate() throws an error
  2. in here because template is null or undefined when it's expected to not be null or undefined

Or wait...would loadSmsTemplate() just not resolve if there was an error 🧐

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's still fine? I the function goes wrong the we show the error from the server and the locale doesn't change right?

<Id value={'{{project}}'}>{'{{project}}'}</Id>
<Id value={'{{redirect}}'}>{'{{redirect}}'}</Id>
</EmailTemplate>
</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems all of these email templates are the same except for the type string passed to loadEmailTemplate(). If that's the case, can we simplify this in any way to reduce the duplicate code?

Copy link
Contributor

Choose a reason for hiding this comment

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

If I remember correctly there were plans for them to be different from one another, hence why the repetition 🙃

package.json Outdated
"@analytics/google-analytics": "^1.0.5",
"@appwrite.io/console": "0.1.0",
"@appwrite.io/pink": "^0.0.6-rc.10",
"@appwrite.io/console": "npm:@aw-labs/sdk-console-smtp@^0.1.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to change this before merge, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes :)

Copy link
Contributor

Choose a reason for hiding this comment

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

<CollapsibleItem
bind:open={smsLoginOpen}
on:click={(e) => {
e.preventDefault();
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is preventDefault needed?

Copy link
Contributor

Choose a reason for hiding this comment

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

If I remember correctly, it's because otherwise the collapsible wouldn't open correctly

Copy link
Contributor

Choose a reason for hiding this comment

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

a comment might be useful then

Copy link
Contributor

Choose a reason for hiding this comment

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

a comment might be useful then

Agreed!

Comment on lines +18 to +20
const timeout = setTimeout(() => {
loading = true;
}, 1);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this needed?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's because Chen wanted to show the loader only if the request took more than 1 second

Copy link
Contributor

Choose a reason for hiding this comment

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

Should be 1000 instead then, no?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oops 🤣

Copy link
Contributor

@stnguyen90 stnguyen90 left a comment

Choose a reason for hiding this comment

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

See previous comments.

class="link"
href="/#">here</a
>.
<!-- TODO: add link to docs -->
Copy link
Contributor

Choose a reason for hiding this comment

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

can we add the link ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we have it yet

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants