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

Records with different terms and tabs #62

Merged
merged 6 commits into from
May 9, 2024
Merged

Conversation

IlievaDayana
Copy link
Contributor

No description provided.

@IlievaDayana IlievaDayana self-assigned this Apr 16, 2024
@IlievaDayana IlievaDayana requested a review from venzovisa April 16, 2024 06:15
@@ -57,7 +58,7 @@ const Assistant = ({ message, itemId }: AssistantProps) => {
<Media
e2e="assistant-iframe"
key={uuidV4()}
title={it[it.type]?.title||''}
title={it[it.type]?.title || ''}
background={`url(https://img.youtube.com/vi/${extractVideoCode(it[it.type]?.url)}/hqdefault.jpg)`}
url={extractVideoCode(it[it.type]?.url || 'https://www.youtube.com/embed/g4B8Dhl4pxY')}
type={Definition.video}
Copy link
Contributor

Choose a reason for hiding this comment

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

@IlievaDayana I don't see any fallback for the background similar to the one for url attribute. Can you provide something?

@@ -142,6 +143,7 @@ const chatMiddleware: Middleware = (store) => (next) => (action) => {
}

if (resendMessage.match(action) && !('$isSync' in action)) {
// @ts-ignore must understand why the action is of type never??
handleMessageResending(action.payload);
}
Copy link
Contributor

@venzovisa venzovisa Apr 16, 2024

Choose a reason for hiding this comment

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

@IlievaDayana What happens if you check existence of payload property in action before use and what is actual type of Action from Redux toolkit? You can try to define interface that extends Action and put required properties in it then pass it for chatMiddleware parameter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i've tried adding type check also property check and both also, but it did not work?
Action where T stands for template, maybe i should describe it somehow.

@IlievaDayana IlievaDayana added the enhancement New feature or request label Apr 30, 2024
term,
threadId: threadId,
},
]);
});

Copy link
Contributor

@venzovisa venzovisa Apr 30, 2024

Choose a reason for hiding this comment

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

@IlievaDayana I see a lot of duplicate objects. You can create factory function that produces mocked objects and call it wherever needed:

const generateStreamingData = (data: Partial<StreamingData>) => ({
          type: 'text',
          text: '',
          sequence: 1,
          id: '4216d0a8-2621-499c-8a65-70191e31ee5a',
          term: '',
          threadId: faker.random.uuid(),
          ...data
});

errors: [],
term: term,
threadId: threadId,
});
Copy link
Contributor

Choose a reason for hiding this comment

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

@IlievaDayana you can pass term and threadId only once for short version.

{
...
 term,
 threadId
}

}

return 0;
};

Copy link
Contributor

Choose a reason for hiding this comment

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

@IlievaDayana Do you plan to add tests for these methods?

sent: record.sent || true,
groupId: record.groupId || '',
itemId: itemId
}}
Copy link
Contributor

Choose a reason for hiding this comment

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

@IlievaDayana could be passed only once:

{
    ...
    itemId
}

@IlievaDayana IlievaDayana merged commit d60394f into master May 9, 2024
1 check passed
@IlievaDayana IlievaDayana deleted the term-tab-in-sync branch May 9, 2024 12:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants