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

Cs 7762 create and implement subclass in activity edit and embedded mode #2032

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

lucaslyl
Copy link
Contributor

@lucaslyl lucaslyl self-assigned this Jan 13, 2025
Copy link

github-actions bot commented Jan 13, 2025

Host Test Results

    1 files  ±0      1 suites  ±0   20m 56s ⏱️ -11s
726 tests ±0  724 ✔️ ±0  2 💤 ±0  0 ±0 
731 runs  ±0  729 ✔️ ±0  2 💤 ±0  0 ±0 

Results for commit d7d51a4. ± Comparison against base commit fa0e1e4.

♻️ This comment has been updated with latest results.

Comment on lines +318 to +326
<Pill
style={{htmlSafe
(concat
'background-color: '
@model.status.colorScheme.backgroundColor
'; border-color: transparent; font-weight: 600; font-size: 11px;'
)
}}
>{{@model.status.label}}</Pill>
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's ease down on these style injections. You can use @pillBackgroundColor property here. For the rest use a class name and write the css.

{
index: 0,
icon: Phone,
label: 'Customer Call',
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this should just be 'Call' or 'Phone Call'. If the contact has customer label, we can determine that it's a customer call.

Comment on lines +98 to +107
this.args.model.email = freshEmail;
this.args.model.meeting = freshMeeting;
break;
case 'Email':
this.args.model.call = freshCall;
this.args.model.meeting = freshMeeting;
break;
case 'Meeting':
this.args.model.call = freshCall;
this.args.model.email = freshEmail;
Copy link
Contributor

Choose a reason for hiding this comment

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

can't these just be set to undefined or null?

Comment on lines +10 to +11
@field callTime = contains(DatetimeField);
@field callDuration = contains(StringField);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we use the date-range field here?

Comment on lines +13 to +21
@field title = contains(StringField, {
computeVia: function (this: Call) {
const phoneStr =
`${this.phoneNumber.countryCode}-${this.phoneNumber.number}` ?? '';
const timeStr = this.callTime?.toLocaleString() ?? '';
const durationStr = this.callDuration ?? '';
return `${phoneStr} - ${timeStr} - ${durationStr}`;
},
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Why don't we let the user decide on the title for each of the activity types? The empty strings or nulls look strange.

Comment on lines +10 to +11
@field emailContent = contains(StringField);
@field sentTime = contains(DatetimeField);
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't sentTime be the timestamp? If it's meant to be for scheduling, maybe a clearer field name would be better. Is this the actual content of the email? If it is necessary, maybe it should be a markdown field.

Comment on lines +10 to +11
@field startTime = contains(DatetimeField);
@field endTime = contains(DatetimeField);
Copy link
Contributor

Choose a reason for hiding this comment

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

How about the date range field here?

import { Email } from './activity-type/email';
import { Meeting } from './activity-type/meeting';
import type IconComponent from '@cardstack/boxel-icons/captions';

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 organize all these imports above better?

{{#if this.activity}}
<FieldContainer @vertical={{true}} class='activity-details'>
{{#if (eq this.activity.label 'Customer Call')}}
<@fields.call @format='edit' />
Copy link
Contributor

Choose a reason for hiding this comment

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

These will probably be edit fields by default since this is an edit template

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants