feat(cc-digital-channel): add digital channel package#586
feat(cc-digital-channel): add digital channel package#586Shreyas281299 merged 29 commits intowebex:nextfrom
Conversation
| @@ -0,0 +1,13 @@ | |||
| // Declare custom HTML elements used by the Webex Engage components | |||
There was a problem hiding this comment.
Since we wont require md-theme, we can get rid of this file as well
There was a problem hiding this comment.
Not sure if this comment is still applicable... but if so, we can remove this file.
There was a problem hiding this comment.
Lets check this please.
There was a problem hiding this comment.
We do require md-theme. It is required. Since it's a old momentum. We need to add the types explicitly.
| import {mockTask, mockCC} from '@webex/test-fixtures'; | ||
|
|
||
| // Mock mobx-react-lite to make observer work properly in tests | ||
| jest.mock('mobx-react-lite', () => ({ |
There was a problem hiding this comment.
Can we avoid this, we wanna make sure the store changes are being recevied by the widget
There was a problem hiding this comment.
We are still mocking the store, can we remove that
| it('should successfully load and initialize real Engage component without errors', () => { | ||
| // This test proves we can test with the real Engage component | ||
| expect(() => { | ||
| render(<DigitalChannels {...mockProps} />); |
There was a problem hiding this comment.
Can we check something in the dom to ensure the widget is loaded
There was a problem hiding this comment.
Same here, just expecting no error wont gauranteee the widget has actually reached the dom
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
| import {createRoot} from 'react-dom/client'; | ||
|
|
||
| // Initialize AGENTX_SERVICE before any imports that might need it | ||
| window['AGENTX_SERVICE'] = {}; // Required by engage widgets |
There was a problem hiding this comment.
lets revisit this after Rajeev comeback if he can fix it
| * | ||
| * @returns The normalized region string (e.g., 'produs1', 'intgus1') or undefined if parsing fails | ||
| */ | ||
| export function extractRegionFromRtmsDomain(domain: string): string | undefined { |
There was a problem hiding this comment.
Lets revisit and discuss with team, how to do it more effectively
adhmenon
left a comment
There was a problem hiding this comment.
Overall code is LGTM.
Suggested few code changes.
Again, main thing to discuss is the getDataCenter method, ideally we should compute it in the SDK.
Have discussed with Ravi over the call and we are aware of the current method workaround.
| @@ -0,0 +1,35 @@ | |||
| import {ITask} from '@webex/cc-store'; | |||
|
|
|||
| export interface UseDigitalChannelsInitProps { | |||
There was a problem hiding this comment.
Nit: Maybe we can rename this and remove the Use part from the names.
There was a problem hiding this comment.
I agree, Im assuming this is for hook. We were using this naming convention in web-client, in this repo we just use the work hook
| export interface UseDigitalChannelsInitProps { | |
| export interface DigitalChanelsHooksProps { |
| import {DigitalChannelsComponent} from './DigitalChannelsComponent'; | ||
| import {DigitalChannelsProps} from './digital-channels.types'; | ||
|
|
||
| const DigitalChannelsInternal: React.FunctionComponent<DigitalChannelsProps> = observer(({currentTheme}) => { |
There was a problem hiding this comment.
Is it possible to move this to another file just for organisation perspective perhaps.
| @@ -0,0 +1,13 @@ | |||
| // Declare custom HTML elements used by the Webex Engage components | |||
There was a problem hiding this comment.
Not sure if this comment is still applicable... but if so, we can remove this file.
| @@ -0,0 +1,10 @@ | |||
| // This config is to do type checking in our files while running tests. | |||
There was a problem hiding this comment.
Maybe we can remove some of these comments.
| @@ -0,0 +1,35 @@ | |||
| import {ITask} from '@webex/cc-store'; | |||
|
|
|||
| export interface UseDigitalChannelsInitProps { | |||
There was a problem hiding this comment.
I agree, Im assuming this is for hook. We were using this naming convention in web-client, in this repo we just use the work hook
| export interface UseDigitalChannelsInitProps { | |
| export interface DigitalChanelsHooksProps { |
|
|
||
| return ( | ||
| <div> | ||
| <md-theme id="app-theme" theme="momentumV2" {...(isDarkTheme ? {darktheme: true} : {lighttheme: true})}> |
There was a problem hiding this comment.
Did we confirm if this is required? To use widgets we have asked dev to wrap the app in Theme and Icon Provider, so if a developer does that and here we are again wrapping the component that mean the Engage widget is wrapped twice. Just wanna ensure this doesnt break anything. Maybe its worthwhile to check with momentum team about this, We are wrapping a web-component in a react component.
There was a problem hiding this comment.
Confirmed with Digital Channels team. They strictly require this. Engage Widget should be wrapped inside the md-theme.
| @@ -0,0 +1,13 @@ | |||
| // Declare custom HTML elements used by the Webex Engage components | |||
There was a problem hiding this comment.
Lets check this please.
| // Initialize the digital channels app only once per session | ||
| if (!isDigitalChannelsInitialized) { | ||
| logger.log( | ||
| `[DIGITAL_CHANNELS_INIT] 🚀 Starting Digital Channels initialization for the FIRST TIME (dataCenter: ${dataCenter})...`, |
There was a problem hiding this comment.
Can we not have emogis in logs.
| import {mockTask, mockCC} from '@webex/test-fixtures'; | ||
|
|
||
| // Mock mobx-react-lite to make observer work properly in tests | ||
| jest.mock('mobx-react-lite', () => ({ |
There was a problem hiding this comment.
We are still mocking the store, can we remove that
| it('should successfully load and initialize real Engage component without errors', () => { | ||
| // This test proves we can test with the real Engage component | ||
| expect(() => { | ||
| render(<DigitalChannels {...mockProps} />); |
There was a problem hiding this comment.
Same here, just expecting no error wont gauranteee the widget has actually reached the dom
| it('should demonstrate minimal mocking approach', () => { | ||
| // This test suite demonstrates that we only needed to mock: | ||
| // 1. AGENTX_SERVICE global (minimal requirement) | ||
| // 2. @webex/cc-store (external dependency) | ||
| // 3. mobx-react-lite observer (to simplify MobX in tests) | ||
| expect(true).toBe(true); // Placeholder assertion |
There was a problem hiding this comment.
This test doesnt have anything
There was a problem hiding this comment.
Basic UTs have now been added. We have a separate ticket to cover UTs for all components related to the Digital Channel feature.
Jira ticket: https://jira-eng-sjc12.cisco.com/jira/browse/CAI-7458
There was a problem hiding this comment.
I see we have reduced the file, but still this would need to be done to use digital cahnnel?
No longer valid. Now getting the dataCenter value from the Agent Profile |
COMPLETES #https://jira-eng-sjc12.cisco.com/jira/browse/CAI-7419
Vidcast Link: https://app.vidcast.io/share/a82fefc6-9482-429d-bb23-29f74c7d6183
This pull request addresses
This PR is to consume engage npm packag and expose the digital widget as part of cc-widgets
< DESCRIBE THE CONTEXT OF THE ISSUE >
consumed "@webex-engage/wxengage-conversations": "^1.0.2", added yarnrc t o have cisco repository config and token.
by making the following changes
Created a new package @webex/cc-digital-channels and consumed "@webex-engage/wxengage-conversations": "^1.0.2"
< DESCRIBE YOUR CHANGES >
Change Type
The following scenarios were tested
< ENUMERATE TESTS PERFORMED, WHETHER MANUAL OR AUTOMATED >
The GAI Coding Policy And Copyright Annotation Best Practices
Checklist before merging
Make sure to have followed the contributing guidelines before submitting.