-
Notifications
You must be signed in to change notification settings - Fork 22
Conversation UI #81
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
Conversation UI #81
Conversation
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/nylas/components/E5PCF2u5hd8nuAqf3udEXk9gE6jM |
This pull request has been linked to Shortcut Story #69383: Conversation Component UI Updates. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple comments/questions remaining, and some type-check failures worth addressing before merge please
</button> | ||
{/if} | ||
{#if headerExpanded} | ||
{#each reply.to.splice(1, reply.to.length - 1) as contact} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you expand on what this is doing? Alternatively: set this as a variable and each
over that variable, so we'd have a name like "uniqueParticipants" or something equivalent
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll note that I have a slight aversion to using splice in template logic -- it is mutative and sometimes prone to side-effects. I wonder if there's another way here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right I should be using slice
! All I'm doing here is grabbing the rest of the reply.to
array. The first entry is shown above, when the dropdown button is clicked they need to all be shown
<header class="mobile" class:expanded={headerExpanded}>
<span>to: {reply.to[0]?.email}</span> <!-- First email -->
{#if headerExpanded}
{#each reply.to.slice(1) as contact} <!-- Rest of them -->
<span>to: {contact.email}</span>
commons/src/methods/datetime.ts
Outdated
return date | ||
.toLocaleTimeString() | ||
.replaceAll(/:\d{1,2} /g, "") | ||
.replaceAll(/.m./g, "m"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can do
.toLocaleTimeString([], {
timeStyle: "short",
})
here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the /.m replace doing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ou very cool thanky ou!!
the replace gets rid of the periods
date.toLocaleTimeString([], {timeStyle: "short"})
> "11:06 a.m."
.replaceAll(/.m./g, "m");
> "11:06 am"
I'll simplyfy to
.replaceAll(/\./g, "")
.toLocaleTimeString([], { timeStyle: "short" }) | ||
.replaceAll(/\./g, ""); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix for above is to upgrade typescript microsoft/TypeScript#38266 (comment)
a55c975
to
9420624
Compare
0871e57
to
9420624
Compare
Todo
Readiness checklist
License
I confirm that this contribution is made under the terms of the MIT license and that I have the authority necessary to make this contribution on behalf of its copyright owner.