-
Notifications
You must be signed in to change notification settings - Fork 61
feat(cc-widgets): added-address-book #537
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
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
cmullenx
left a comment
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 will need to add props to show/hide consult/transfer to address book and show/hide transfer to entry point based on feature flags that you will receive from our app.
...cc-components/src/components/task/CallControl/CallControlCustom/consult-transfer-popover.tsx
Show resolved
Hide resolved
...cc-components/src/components/task/CallControl/CallControlCustom/consult-transfer-popover.tsx
Outdated
Show resolved
Hide resolved
...cc-components/src/components/task/CallControl/CallControlCustom/consult-transfer-popover.tsx
Outdated
Show resolved
Hide resolved
packages/contact-center/cc-components/src/components/task/CallControl/call-control.styles.scss
Outdated
Show resolved
Hide resolved
packages/contact-center/cc-components/src/components/task/task.types.ts
Outdated
Show resolved
Hide resolved
...-components/tests/components/task/CallControl/CallControlCustom/consult-transfer-popover.tsx
Show resolved
Hide resolved
...cc-components/src/components/task/CallControl/CallControlCustom/consult-transfer-popover.tsx
Outdated
Show resolved
Hide resolved
feat(cc-widgets): added-basic-playwright-for-address-book
...cc-components/src/components/task/CallControl/CallControlCustom/call-control-custom.utils.ts
Outdated
Show resolved
Hide resolved
...mponents/src/components/task/CallControl/CallControlCustom/consult-transfer-popover-hooks.ts
Show resolved
Hide resolved
...mponents/src/components/task/CallControl/CallControlCustom/consult-transfer-popover-hooks.ts
Outdated
Show resolved
Hide resolved
| apiParams.search = search; | ||
| } | ||
|
|
||
| logger?.info(`CC-Components: Loading ${categoryName} - page: ${currentPage}, search: "${search}"`); |
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.
- Is logger optional? If not, we can remove the null check from here. Applicable everywhere
- We can add the context containing the file and method name in the same way we have at all the places. Applicable everywhere
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.
- In this case, we want logger to be optional as users need not specify a logger when they use our widgets or SDK...
- I will make that change 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.
- I thought this is our logging which gets uploaded to mats. If that's the case, we can't have this optional. Correct me if I am wrong
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 the rationale behind this was since we use these components in our tests (both unit and e2e) - sometimes we don't provide the logger, hence it was made optional...
Again, I am alright changing it to compulsory but that not sure how it would affect tests.
Perhaps we can discuss this with Shreyas also.
...mponents/src/components/task/CallControl/CallControlCustom/consult-transfer-popover-hooks.ts
Outdated
Show resolved
Hide resolved
...mponents/src/components/task/CallControl/CallControlCustom/consult-transfer-popover-hooks.ts
Outdated
Show resolved
Hide resolved
...cc-components/src/components/task/CallControl/CallControlCustom/consult-transfer-popover.tsx
Show resolved
Hide resolved
| getEntryPoints = async (params?: EntryPointSearchParams): Promise<EntryPointListResponse> => { | ||
| try { | ||
| const response: EntryPointListResponse = await this.store.cc.getEntryPoints(params); | ||
| return response; | ||
| } catch (error) { | ||
| console.error('Error fetching entry points:', error); | ||
| return Promise.reject(error); | ||
| } | ||
| }; |
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.
Do we need to create this method? This is just one line logic
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.
Yes, I feel so - we can catch any errors here in this method and it makes it uniform.
I feel we should retain this.
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.
console.error('Error fetching entry points:', error);Please use the logger here and also add proper logging at the top and for the success as well- We can simply throw the error from the catch block instead of calling the promise.reject
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.
Added here as well
| getAddressBookEntries = async (params?: AddressBookEntrySearchParams): Promise<AddressBookEntriesResponse> => { | ||
| try { | ||
| const response: AddressBookEntriesResponse = await this.store.cc.addressBook.getEntries(params ?? {}); | ||
| return response; | ||
| } catch (error) { | ||
| console.error('Error fetching address book entries:', error); | ||
| return Promise.reject(error); | ||
| } | ||
| }; |
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.
Same 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.
As mentioned above, I feel it makes sense to keep it as we cna do error handling and it is uniform.
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.
Same here. Let's improve the logging and error throwing
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.
Added here also.. not sure why it's not showing outdated.. code changes are there in the code tabs.
...mponents/src/components/task/CallControl/CallControlCustom/consult-transfer-popover-hooks.ts
Outdated
Show resolved
Hide resolved
rarajes2
left a comment
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.
LGTM
...mponents/src/components/task/CallControl/CallControlCustom/consult-transfer-popover-hooks.ts
Outdated
Show resolved
Hide resolved
...mponents/src/components/task/CallControl/CallControlCustom/consult-transfer-popover-hooks.ts
Outdated
Show resolved
Hide resolved
...cc-components/src/components/task/CallControl/CallControlCustom/consult-transfer-popover.tsx
Show resolved
Hide resolved
| {renderList( | ||
| queuesData.map((q) => ({id: q.id, name: q.name})), | ||
| (item) => handleQueueSelection(item.id, item.name, onQueueSelect, logger) | ||
| )} |
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.
Ideally we want to simplify this all we should see in presentational is
| {renderList( | |
| queuesData.map((q) => ({id: q.id, name: q.name})), | |
| (item) => handleQueueSelection(item.id, item.name, onQueueSelect, logger) | |
| )} | |
| {renderList(queuesData)} |
We can process the data in utils
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.
As mentioned above - this convolutes things and adds extra complexity to this PR... perhaps we can look at it later on.
...cc-components/src/components/task/CallControl/CallControlCustom/consult-transfer-popover.tsx
Outdated
Show resolved
Hide resolved
packages/contact-center/cc-components/src/components/task/CallControl/call-control.styles.scss
Show resolved
Hide resolved
packages/contact-center/cc-components/src/components/task/CallControl/call-control.styles.scss
Outdated
Show resolved
Hide resolved
...-components/tests/components/task/CallControl/CallControlCustom/call-control-custom.util.tsx
Outdated
Show resolved
Hide resolved
...-components/tests/components/task/CallControl/CallControlCustom/consult-transfer-popover.tsx
Outdated
Show resolved
Hide resolved
...-components/tests/components/task/CallControl/CallControlCustom/consult-transfer-popover.tsx
Outdated
Show resolved
Hide resolved
packages/contact-center/cc-components/tests/components/task/CallControl/call-control.tsx
Outdated
Show resolved
Hide resolved
Shreyas281299
left a comment
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.
Looks good.
# [1.28.0-ccwidgets.118](v1.28.0-ccwidgets.117...v1.28.0-ccwidgets.118) (2025-10-15) ### Features * **cc-widgets:** added-address-book ([#537](#537)) ([0997d10](0997d10))
|
🎉 This PR is included in version 1.28.0-ccwidgets.118 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
COMPLETES #< CAI-7147 >
This pull request addresses
by making the following changes
Vidcast - https://app.vidcast.io/share/2f481e6b-227a-4652-92e9-0f228b48ca03
Change Type
The following scenarios were tested
The GAI Coding Policy And Copyright Annotation Best Practices
Checklist before merging