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

TNO-1136: User can set toning #2226

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Binary file not shown.
2 changes: 1 addition & 1 deletion app/editor/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@
"redux-logger": "3.0.6",
"styled-components": "6.1.11",
"stylis": "4.3.2",
"tno-core": "0.1.139"
"tno-core": "./tno-core-0.1.139.tgz"
},
"devDependencies": {
"@simbathesailor/use-what-changed": "2.0.0",
Expand Down
Binary file added app/editor/tno-core-0.1.139.tgz
Binary file not shown.
8 changes: 4 additions & 4 deletions app/editor/yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -11074,7 +11074,7 @@ __metadata:
sass-extract-loader: 1.1.0
styled-components: 6.1.11
stylis: 4.3.2
tno-core: 0.1.139
tno-core: ./tno-core-0.1.139.tgz
typescript: 4.9.5
languageName: unknown
linkType: soft
Expand Down Expand Up @@ -15258,9 +15258,9 @@ __metadata:
languageName: node
linkType: hard

"tno-core@npm:0.1.139":
"tno-core@file:./tno-core-0.1.139.tgz::locator=mmi-editor-app%40workspace%3A.":
version: 0.1.139
resolution: "tno-core@npm:0.1.139"
resolution: "tno-core@file:./tno-core-0.1.139.tgz::locator=mmi-editor-app%40workspace%3A."
dependencies:
"@elastic/elasticsearch": ^8.13.1
"@fortawesome/free-solid-svg-icons": ^6.4.2
Expand Down Expand Up @@ -15293,7 +15293,7 @@ __metadata:
styled-components: ^6.1.11
stylis: ^4.3.2
yup: ^1.1.1
checksum: 92382ad294654dbbb8f8387726b6a96f889ba4d65ea452560dfc6cc9b1fc663ba26152b569e19ee5007067ee1734af1a75a2c06b57c33d3292cf235e2d8c1775
checksum: a560860593ae77c9d75a13223b2c123473c685451375346927b7b52506e22fa3a64e4dc57669ef30546e241e22fd87f49652539efbd856718864e5d0e1631133
languageName: node
linkType: hard

Expand Down
Binary file not shown.
2 changes: 1 addition & 1 deletion app/subscriber/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@
"sheetjs": "file:packages/xlsx-0.20.1.tgz",
"styled-components": "6.1.11",
"stylis": "4.3.2",
"tno-core": "0.1.139"
"tno-core": "./tno-core-0.1.139.tgz"
},
"devDependencies": {
"@testing-library/jest-dom": "6.4.5",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -215,7 +215,14 @@ export const ContentEditForm = React.forwardRef<HTMLDivElement | null, IContentE
</Col>
<Col className="sentiment">
<Sentiment
value={form.content.tonePools.length ? form.content.tonePools[0].value : undefined}
value={
form.content.versions?.[userId]?.tone !== undefined &&
Copy link
Collaborator

Choose a reason for hiding this comment

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

As far as I can tell tone should never be null

Copy link
Collaborator Author

@areyeslo areyeslo Aug 29, 2024

Choose a reason for hiding this comment

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

First time the tone can be null when the user did not save her/his tone and until tone is saved by the user, then we have a value.

form.content.versions?.[userId]?.tone !== null
? form.content.versions?.[userId]?.tone
: form.content.tonePools.length
? form.content.tonePools[0].value
: undefined
}
showValue
/>
</Col>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
import { Form, Formik } from 'formik';
import React from 'react';
import { useApp } from 'store/hooks';
import { useApp, useLookup } from 'store/hooks';
import { useProfileStore } from 'store/slices';
import {
Col,
ContentTypeName,
FormikSentiment,
IContentModel,
Loading,
Show,
Expand All @@ -12,6 +14,8 @@ import {
Wysiwyg,
} from 'tno-core';

import { sentimentFormSchema } from '../../validation/SentimentFormSchema';

export interface IContentFormProps extends Omit<React.HTMLAttributes<HTMLDivElement>, 'content'> {
/** The content being edited */
content?: IContentModel;
Expand Down Expand Up @@ -43,20 +47,47 @@ export const ContentForm: React.FC<IContentFormProps> = ({
}) => {
const [{ userInfo }] = useApp();
const [{ impersonate }] = useProfileStore();
const [{ tonePools }] = useLookup();

if (!content) return null;

const userId = impersonate?.id ?? userInfo?.id ?? 0;
const isAV = content.contentType === ContentTypeName.AudioVideo;
const versions = content.versions?.[userId] ?? {
byline: content.byline,
headline: content.headline,
summary: '',
body: isAV
? content.isApproved && content.body
? content.body
: content.summary
: content.body,

const sentiment =
content.versions?.[userId]?.tone !== undefined && content.versions?.[userId]?.tone != null
Copy link
Collaborator

Choose a reason for hiding this comment

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

As far as I can tell tone should never be null

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

First time the tone can be null when the user did not save her/his tone and until tone is saved by the user, then we have a value.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If it can be null then your interface is not correct. I would recommend updating the interface if you want to handle null. However, I don't know why you would want both null and undefined.
This is one of the reasons I'm not a fan of javascript.

? content.versions?.[userId]?.tone
: content.tonePools.length
? content.tonePools[0].value
: undefined;

const versions = {
...content.versions?.[userId],
byline: content.versions?.[userId]?.byline ?? content.byline,
headline: content.versions?.[userId]?.headline ?? content.headline,
summary: content.versions?.[userId]?.summary ?? '',
body:
content.versions?.[userId]?.body ??
(isAV ? (content.isApproved && content.body ? content.body : content.summary) : content.body),
tonePools:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are you using a property tonePools here? The model is for IContentVersionModel has a tone property you added.

sentiment !== undefined && sentiment !== null
? [{ value: sentiment, id: tonePools[0].id }]
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is no reason to apply the wrong id to this user's tone`

: [],
};

const handleSentimentChange = (newTonePools: any) => {
const validTonePools = Array.isArray(newTonePools) ? newTonePools : [];
const updatedContent = {
...content,
versions: {
...content.versions,
[userId]: {
...content.versions?.[userId],
tone: validTonePools[0]?.value,
},
},
};
onContentChange?.(updatedContent);
};

return show === 'none' ? null : (
Expand Down Expand Up @@ -152,6 +183,19 @@ export const ContentForm: React.FC<IContentFormProps> = ({
onContentChange?.(values);
}}
/>
<Formik initialValues={versions} validationSchema={sentimentFormSchema} onSubmit={() => {}}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't create a sub-form simply to work around the behavior of the FormikSentiment. Make the component smarter.

{() => (
<Form>
<FormikSentiment
Copy link
Collaborator

Choose a reason for hiding this comment

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

How is this component able to show the current value the user selected when the page is reloaded?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Through using handleSentimentChange callback which is executed when the user clicks on the tone button.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That function saves the value. The component has no way to know the current value. If you refresh the page it wouldn't know how to get the value.

name="tonePools"
options={tonePools}
coloredIcon={true}
onSentimentChange={handleSentimentChange}
required
/>
</Form>
)}
</Formik>
</Show>
</Col>
);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
import { array, number, object } from 'yup';

export const sentimentFormSchema = object().shape({
tonePools: array().of(
object().shape({
value: number().required('Tone value is required'),
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't believe tone is required. Did Scott ask for this?

}),
),
});
18 changes: 17 additions & 1 deletion app/subscriber/src/store/hooks/lookup/useLookup.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import {
ITopicScoreRuleModel,
IUserModel,
StorageKeys,
useApiEditorTonePools,
useApiSubscriberCache,
useApiSubscriberMinisters,
} from 'tno-core';
Expand All @@ -44,6 +45,7 @@ export const useLookup = (): [ILookupState, ILookupController] => {
const cache = useApiSubscriberCache();
const lookups = useApiLookups();
const ministers = useApiSubscriberMinisters();
const tonePools = useApiEditorTonePools();

const controller = React.useMemo(
() => ({
Expand Down Expand Up @@ -156,13 +158,27 @@ export const useLookup = (): [ILookupState, ILookupController] => {
'lookup',
);
},
getTonePools: async () => {
return await fetchIfNoneMatch<ITonePoolModel[]>(
StorageKeys.TonePools,
dispatch,
(etag) => tonePools.getTonePools(etag),
(results) => {
const values = results ?? [];
store.storeTonePools(values);
return values;
},
true,
'lookup',
);
},
init: async () => {
// TODO: Handle failures
await controller.getLookups();
store.storeIsReady(true);
},
}),
[cache, dispatch, lookups, ministers, store],
[cache, dispatch, lookups, ministers, tonePools, store],
);

return [state, controller];
Expand Down
Binary file added app/subscriber/tno-core-0.1.139.tgz
Binary file not shown.
8 changes: 4 additions & 4 deletions app/subscriber/yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -10697,7 +10697,7 @@ __metadata:
sheetjs: "file:packages/xlsx-0.20.1.tgz"
styled-components: 6.1.11
stylis: 4.3.2
tno-core: 0.1.139
tno-core: ./tno-core-0.1.139.tgz
typescript: 4.9.5
languageName: unknown
linkType: soft
Expand Down Expand Up @@ -14729,9 +14729,9 @@ __metadata:
languageName: node
linkType: hard

"tno-core@npm:0.1.139":
"tno-core@file:./tno-core-0.1.139.tgz::locator=mmi-subscriber-app%40workspace%3A.":
version: 0.1.139
resolution: "tno-core@npm:0.1.139"
resolution: "tno-core@file:./tno-core-0.1.139.tgz::locator=mmi-subscriber-app%40workspace%3A."
dependencies:
"@elastic/elasticsearch": ^8.13.1
"@fortawesome/free-solid-svg-icons": ^6.4.2
Expand Down Expand Up @@ -14764,7 +14764,7 @@ __metadata:
styled-components: ^6.1.11
stylis: ^4.3.2
yup: ^1.1.1
checksum: 92382ad294654dbbb8f8387726b6a96f889ba4d65ea452560dfc6cc9b1fc663ba26152b569e19ee5007067ee1734af1a75a2c06b57c33d3292cf235e2d8c1775
checksum: a560860593ae77c9d75a13223b2c123473c685451375346927b7b52506e22fa3a64e4dc57669ef30546e241e22fd87f49652539efbd856718864e5d0e1631133
languageName: node
linkType: hard

Expand Down
5 changes: 5 additions & 0 deletions libs/net/entities/Models/ContentVersion.cs
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,10 @@ public class ContentVersion
/// </summary>
public bool IsPrivate { get; set; } = false;

/// <summary>
/// get/set - Tone for the content
/// </summary>
public int? Tone {get;set;}

#endregion

Expand All @@ -82,6 +86,7 @@ public ContentVersion(Content content, User owner)
this.Body = content.Body;
this.SourceUrl = content.SourceUrl;
this.IsPrivate = content.IsPrivate;
this.Tone = content.TonePools.Count > 0 ? (int?)content.TonePools[0].Id : null;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This incorrect, it should be the TonePoolsManyToMany.FirstOrDefault().Value not the TonePool[0].Id.

}

/// <summary>
Expand Down
37 changes: 29 additions & 8 deletions libs/npm/core/src/components/formik/sentiment/FormikSentiment.tsx
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { getIn, useFormikContext } from 'formik';
import React from 'react';
import { FaFrown, FaMeh, FaSmile } from 'react-icons/fa';

import FaceFrownOpen from '../../../assets/face-frown-open.svg';
import FaceGrinWide from '../../../assets/face-grin-wide.svg';
Expand All @@ -17,10 +18,14 @@ export interface IFormikSentimentProps<T> {
label?: string;
/** Sentiment options. */
options: ITonePoolModel[];
/** Sentiment icon colored */
coloredIcon?: boolean;
/** The name of the default tone pool */
defaultTonePoolName?: string;
/** The id of the default tone pool */
defaultTonePoolId?: number;
/** Update upper form */
onSentimentChange?: (tonePools: any) => void;
/** Whether this field is required. */
required?: boolean;
}
Expand All @@ -34,8 +39,10 @@ export const FormikSentiment = <T extends object>({
name,
label = 'Sentiment',
options,
coloredIcon = false,
defaultTonePoolId,
defaultTonePoolName = 'Default',
onSentimentChange,
...rest
}: IFormikSentimentProps<T>) => {
const { values, setFieldValue, touched, errors } = useFormikContext<T>();
Expand All @@ -44,8 +51,10 @@ export const FormikSentiment = <T extends object>({
name,
label,
options,
coloredIcon,
defaultTonePoolId,
defaultTonePoolName,
onSentimentChange,
...rest,
};
const defaultTonePool = options.find(
Expand All @@ -68,11 +77,23 @@ export const FormikSentiment = <T extends object>({

const determineIndicator = (option: number) => {
if (option === 5) {
return <img alt={option.toString()} src={FaceGrinWide} />;
return coloredIcon ? (
<FaSmile className="tone-icon" color="#59E9BE" />
) : (
<img alt={option.toString()} src={FaceGrinWide} />
);
} else if (option === 0) {
return <img alt={option.toString()} src={FaceMeh} />;
return coloredIcon ? (
<FaMeh className="tone-icon" color="#F1C02D" />
) : (
<img alt={option.toString()} src={FaceMeh} />
);
} else if (option === -5) {
return <img alt={option.toString()} src={FaceFrownOpen} />;
return coloredIcon ? (
<FaFrown className="tone-icon" color="#EB8585" />
) : (
<img alt={option.toString()} src={FaceFrownOpen} />
);
} else {
return <span className="blank">&nbsp;</span>;
}
Expand All @@ -92,11 +113,11 @@ export const FormikSentiment = <T extends object>({
key={option}
onClick={() => {
setActive(option);
setFieldValue(
name.toString(),
!!defaultTonePool ? [{ ...defaultTonePool, value: option }] : [],
true,
);
const updatedTonePools = !!defaultTonePool
? [{ ...defaultTonePool, value: option }]
: [];
setFieldValue(name.toString(), updatedTonePools, true);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is going to modify the the formik context field and also call the onSentimentChange. You can pass the valid name to the component to control what is being updated. Or you can choose to only perform the onSentimentChange if it has been provided. Doing both is a bad component.

onSentimentChange?.(updatedTonePools);
}}
>
{option}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,4 +50,10 @@ export const FormikSentiment = styled.div.attrs<IFormikSentimentProps<any>>(({ r
color: red;
}
}

.tone-icon {
font-size: 1.4rem;
margin-bottom: 0rem;
margin-left: 0.5rem;
}
`;
Original file line number Diff line number Diff line change
Expand Up @@ -9,4 +9,5 @@ export interface IContentVersionModel extends IAuditColumnsModel {
page?: string;
summary?: string;
body?: string;
tone?: number;
}
Loading