feat(cc-components): setup and move user state sample ui comp #359
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request introduces a new package Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (9)
packages/contact-center/user-state/src/user-state/index.tsx (1)
16-16: Consider explicit prop passing instead of prop spreading.While spreading props works, explicitly passing props improves maintainability and makes prop dependencies clearer:
- return <UserStateComponent {...props}/>; + return ( + <UserStateComponent + idleCodes={props.idleCodes} + setAgentStatus={props.setAgentStatus} + isSettingAgentStatus={props.isSettingAgentStatus} + errorMessage={props.errorMessage} + elapsedTime={props.elapsedTime} + currentState={props.currentState} + /> + );packages/contact-center/cc-components/webpack.config.js (2)
17-17: Fix incorrect comment about output filename.The comment is incorrect as the filename is actually
[name].js, notindex.js:- filename: '[name].js', // Set the output filename to index.js + filename: '[name].js', // Output files will be named wc.js and index.js
15-19: Consider adding source map configuration for better debugging.For better debugging experience in development, consider adding source map configuration:
output: { path: path.resolve(__dirname, 'dist'), filename: '[name].js', libraryTarget: 'commonjs2', + devtoolModuleFilenameTemplate: 'webpack://[namespace]/[resource-path]?[loaders]' }, + devtool: process.env.NODE_ENV === 'development' ? 'source-map' : false,Note: Based on the retrieved learning, bundle size optimizations are handled in separate tickets, so we'll skip those suggestions.
packages/contact-center/cc-components/src/components/user-state/user-state.tsx (4)
10-15: Consider extracting formatTime to a shared utility.The
formatTimefunction is well-implemented but could be useful across other components. Consider moving it to a shared utilities file for better code reuse.- const formatTime = (time: number): string => { - const hours = Math.floor(time / 3600); - const minutes = Math.floor((time % 3600) / 60); - const seconds = time % 60; - return `${hours.toString().padStart(2, '0')}:${minutes.toString().padStart(2, '0')}:${seconds.toString().padStart(2, '0')}`; - };Create a new file
src/utils/time.ts:export const formatTime = (time: number): string => { const hours = Math.floor(time / 3600); const minutes = Math.floor((time % 3600) / 60); const seconds = time % 60; return `${hours.toString().padStart(2, '0')}:${minutes.toString().padStart(2, '0')}:${seconds.toString().padStart(2, '0')}`; };
23-23: Remove empty div.This empty div with flex styles doesn't serve any purpose.
- <div style={{ display: 'flex', flexDirection: 'column', flexGrow: 1 }} />
29-33: Optimize the event handler implementation.The filter operation inside the onChange handler could be optimized by using find instead of filter, as we only need the first matching item.
- const code = idleCodes?.filter(code => code.id === event.target.value)[0]; + const code = idleCodes?.find(code => code.id === event.target.value); setAgentStatus(code);
49-49: Move error message styling to SCSS.For consistency, move the inline error styling to the SCSS file.
- errorMessage && <div style={{color: 'red'}}>{errorMessage}</div> + errorMessage && <div className="error-message">{errorMessage}</div>Add to user-state.scss:
.error-message { color: red; }packages/contact-center/cc-components/tests/components/user-state/user-state.tsx (1)
57-60: Enhance test coverage for elapsed time styling.Consider adding tests for the following scenarios:
- Verify that elapsed time formatting works with edge cases (0 seconds, large numbers)
- Test that the disabled state properly prevents user interactions
it('should handle edge cases in elapsed time formatting', () => { const { rerender } = render(<UserStatePresentational {...defaultProps} elapsedTime={0} />); expect(screen.getByText('00:00:00')).toBeInTheDocument(); rerender(<UserStatePresentational {...defaultProps} elapsedTime={359999} />); // 99:59:59 expect(screen.getByText('99:59:59')).toBeInTheDocument(); });packages/contact-center/cc-components/package.json (1)
60-61: Address the TODO comment in Jest configuration.The comment indicates that
passWithNoTestscan be removed when tests are added. Since tests have been implemented, this configuration can be removed.- "//": "We can remove this when we have tests", - "passWithNoTests": true,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (12)
packages/contact-center/cc-components/.babelrc.js(1 hunks)packages/contact-center/cc-components/package.json(1 hunks)packages/contact-center/cc-components/src/components/user-state/user-state.scss(1 hunks)packages/contact-center/cc-components/src/components/user-state/user-state.tsx(1 hunks)packages/contact-center/cc-components/src/index.ts(1 hunks)packages/contact-center/cc-components/src/wc.ts(1 hunks)packages/contact-center/cc-components/tests/components/user-state/user-state.tsx(2 hunks)packages/contact-center/cc-components/tsconfig.json(1 hunks)packages/contact-center/cc-components/webpack.config.js(1 hunks)packages/contact-center/user-state/package.json(1 hunks)packages/contact-center/user-state/src/user-state/index.tsx(2 hunks)packages/contact-center/user-state/src/user-state/user-state.presentational.tsx(0 hunks)
💤 Files with no reviewable changes (1)
- packages/contact-center/user-state/src/user-state/user-state.presentational.tsx
✅ Files skipped from review due to trivial changes (3)
- packages/contact-center/cc-components/.babelrc.js
- packages/contact-center/cc-components/tsconfig.json
- packages/contact-center/cc-components/src/components/user-state/user-state.scss
🧰 Additional context used
📓 Learnings (1)
packages/contact-center/cc-components/webpack.config.js (1)
Learnt from: Shreyas281299
PR: webex/widgets#345
File: packages/contact-center/cc-widgets/webpack.config.js:6-12
Timestamp: 2024-12-04T07:40:14.028Z
Learning: In the webex-widgets project, optimizations such as reducing bundle size and improving webpack configurations in files like `packages/contact-center/cc-widgets/webpack.config.js` are handled in separate tickets.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: setup_test_release
🔇 Additional comments (4)
packages/contact-center/cc-components/src/wc.ts (1)
1-8: Well-implemented web component registration!Good practices observed:
- Safe custom element registration with duplicate check
- Proper naming convention with 'component-' prefix
- Clean usage of r2wc for React to Web Component conversion
packages/contact-center/cc-components/src/components/user-state/user-state.tsx (1)
1-8: LGTM! Clean imports and proper TypeScript usage.The component is well-structured with proper type definitions and imports.
packages/contact-center/user-state/package.json (1)
22-22: LGTM! Proper workspace dependency configuration.The addition of
@webex/cc-componentsas a workspace dependency is correct and aligns with the monorepo structure.packages/contact-center/cc-components/package.json (1)
41-53: Review polyfill dependencies.Several browser polyfills are included in devDependencies. Please verify if all of these are necessary for the build process:
- crypto-browserify
- os-browserify
- querystring-es3
- stream-browserify
- url
- util
- vm-browserify
Consider removing unused polyfills to reduce package size and build complexity.
|
Hi @Kesari3008,
We are doing it for User State as an example of how this would go. So, for other widgets, anyone who picks the UI work will take care
Done. Please check. |
package.json
Outdated
| @@ -17,6 +17,7 @@ | |||
| "@semantic-release/git": "^10.0.1", | |||
| "@semantic-release/github": "^11.0.1", | |||
| "@testing-library/react-hooks": "^8.0.1", | |||
| "copy-webpack-plugin": "^6.3.1", | |||
There was a problem hiding this comment.
Remove this. Not necessary
| "@momentum-ui/core": ">=19.16.0", | ||
| "@momentum-ui/icons": ">=8.28.5", | ||
| "@momentum-ui/tokens": ">=1.7.1", | ||
| "@momentum-ui/utils": ">=6.2.15", | ||
| "@momentum-ui/web-components": ">=2.16.16" |
There was a problem hiding this comment.
Add react and react-dom as peer-dependencies
| const props: IUserState = useUserState({ | ||
| idleCodes, | ||
| agentId, | ||
| cc | ||
| cc, | ||
| currentTheme |
There was a problem hiding this comment.
Do not send currentTheme to the hook
There was a problem hiding this comment.
Add todo to investigate if there's a better way to theming
There was a problem hiding this comment.
Done both. Added todo in open items jira
| @@ -20,6 +20,7 @@ class Store implements IStore { | |||
| idleCodes: IdleCode[] = []; | |||
| agentId: string = ''; | |||
| selectedLoginOption: string = ''; | |||
| currentTheme: string = 'light'; | |||
There was a problem hiding this comment.
This should be all caps
| import {StationLogin, UserState, IncomingTask, TaskList, store} from '@webex/cc-widgets'; | ||
|
|
||
| function App() { | ||
| const [isSdkReady, setIsSdkReady] = useState(false); | ||
| const [accessToken, setAccessToken] = useState(''); | ||
| const [isLoggedIn, setIsLoggedIn] = useState(false); | ||
| const themeCheckbox = useRef(null); |
There was a problem hiding this comment.
| const themeCheckbox = useRef(null); | |
| const themeCheckboxRef = useRef(null); |
|
|
||
| import './user-state.scss'; | ||
|
|
||
| const UserStateComponent: React.FunctionComponent<IUserState> = (props) => { |
There was a problem hiding this comment.
Try adding any return type and see if we still require ts ignore
There was a problem hiding this comment.
This is not working. Added Todo to the Open Items Jira
There was a problem hiding this comment.
We should avoid making the component itself an arrow function. It could be normal named function, would help with error trace.
| const {idleCodes,setAgentStatus,isSettingAgentStatus, errorMessage, elapsedTime, currentState, currentTheme} = props; | ||
|
|
||
| const formatTime = (time: number): string => { | ||
| const hours = Math.floor(time / 3600); | ||
| const minutes = Math.floor((time % 3600) / 60); | ||
| const seconds = time % 60; | ||
| return `${hours.toString().padStart(2, '0')}:${minutes.toString().padStart(2, '0')}:${seconds.toString().padStart(2, '0')}`; | ||
| }; |
There was a problem hiding this comment.
This can be a utility function.
| }, | ||
| "dependencies": { | ||
| "@r2wc/react-to-web-component": "2.0.3", | ||
| "@webex/cc-store": "workspace:*" |
There was a problem hiding this comment.
Do we need this dependency here? The presentational layer should not be directly using the outer states
There was a problem hiding this comment.
Yes. We need this for a type import. On bundle, the whole cc-store won't come in as the tree-shaking would happen.
| } | ||
| } | ||
|
|
||
| .dark-theme { |
There was a problem hiding this comment.
Let's create a JIRA to investigate how we should implement the theming part. Keeping performance and code readability in mind
There was a problem hiding this comment.
We have this added as part of Open Items Jira: https://jira-eng-gpk2.cisco.com/jira/browse/SPARK-608946
|
|
||
| import './user-state.scss'; | ||
|
|
||
| const UserStateComponent: React.FunctionComponent<IUserState> = (props) => { |
There was a problem hiding this comment.
We should avoid making the component itself an arrow function. It could be normal named function, would help with error trace.
| }; | ||
|
|
||
| return ( | ||
| <> |
There was a problem hiding this comment.
This seems to be unnecessary. We can remove this fragment
| .elapsedTime-disabled { | ||
| color: #777; /* Darker gray for disabled state */ | ||
| } | ||
| } |
There was a problem hiding this comment.
nitpick: Always leave a blank line at the end of the file
| "@momentum-ui/icons": "8.28.5", | ||
| "@momentum-ui/tokens": "1.7.1", | ||
| "@momentum-ui/utils": "6.2.15", | ||
| "@momentum-ui/web-components": "^2.16.16", |
There was a problem hiding this comment.
We are not consistent with the versioning. Somewhere we are keeping it exact and somewhere we are leaving the room for patches
There was a problem hiding this comment.
For everything else, there's no more development since this is the old repo and only @momentum-ui/web-components is under active development. That is why we allow patching for it.
@rarajes2 - No. Across our repository, you can see that the file names are hyphenated and the folder names are PascalCase. For now, I will match it. I will try to make another PR to match the PascalCase convention |
@rarajes2 - Again, all of the components are arrow functions at the moment. We can discuss as a team and clean it up in another PR |
|
🎉 This PR is included in version 1.28.0-ccconnectors.1 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 1.28.0-ccwidgets.14 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
# [1.28.0-ccconnectors.1](webex/widgets@v1.27.5...v1.28.0-ccconnectors.1) (2025-02-05) ### Bug Fixes * add material to all components ([webex#376](webex#376)) ([de0ca28](webex@de0ca28)) * **bundling:** fixed the multiple React instance and useRef errors ([webex#355](webex#355)) ([473cd4f](webex@473cd4f)) * **call-control:** add-call-control-widget ([webex#362](webex#362)) ([a677f5e](webex@a677f5e)) * **cc-station-login:** material design ui ([webex#377](webex#377)) ([aec7034](webex@aec7034)) * **cc-store:** receive webex on init, add store types ([webex#341](webex#341)) ([9648969](webex@9648969)) * **cc-widgets:** ship-all-widgets-together ([webex#345](webex#345)) ([83d5a37](webex@83d5a37)) * cleanup and using mobx-react-lite ([webex#356](webex#356)) ([0b304c4](webex@0b304c4)) * convert npm to yarn ([webex#234](webex#234)) ([432ed3c](webex@432ed3c)) * **release:** add-publish-step-in-tooling ([webex#334](webex#334)) ([ca32235](webex@ca32235)) * rename agent state to user state ([webex#361](webex#361)) ([fe409db](webex@fe409db)) * **samples:** change samples index html hrefs ([webex#367](webex#367)) ([ff126ab](webex@ff126ab)) * **user-state:** receive agent stateChange event ([webex#350](webex#350)) ([21d6ce7](webex@21d6ce7)) ### Features * **cc-components:** setup and move user state sample ui comp ([webex#359](webex#359)) ([16a44d0](webex@16a44d0)) * **cc-store:** add logger from sdk ([webex#354](webex#354)) ([a62494b](webex@a62494b)) * **cc-widgets:** added Agent-Multi-Login-Alert Feature ([webex#364](webex#364)) ([f7d75ca](webex@f7d75ca)) * **release:** add new branch to circleci ([18f7bec](webex@18f7bec)) * **release:** publish pipeline for wxcc widgets ([webex#324](webex#324)) ([864fb52](webex@864fb52)) * taskList and IncomingTask widgets added ([webex#348](webex#348)) ([ce3a619](webex@ce3a619)) * **user-state:** load and change state, client timer ([webex#347](webex#347)) ([f1ccaeb](webex@f1ccaeb)) * **widget-cc-station-login:** Spark 575845 login widget ([webex#239](webex#239)) ([66b8a20](webex@66b8a20)) * **widgets:** added-relogin-logic ([webex#357](webex#357)) ([94dd415](webex@94dd415)) * **widgets:** shifted-timer-to-worker ([webex#352](webex#352)) ([c06fe9c](webex@c06fe9c))
# [1.28.0-ccwidgets.14](webex/widgets@v1.28.0-ccwidgets.13...v1.28.0-ccwidgets.14) (2025-02-06) ### Bug Fixes * **cc-components:** replace momentum-ui with momentum-design lib ([webex#378](webex#378)) ([b98d76c](webex@b98d76c)) ### Features * **cc-components:** setup and move user state sample ui comp ([webex#359](webex#359)) ([16a44d0](webex@16a44d0)) * **cc-widgets:** added Agent-Multi-Login-Alert Feature ([webex#364](webex#364)) ([f7d75ca](webex@f7d75ca))
Closes Adhoc
This PR addresses
by making the following changes
The following tests were done
elapsedTimeScreenshot & Recording
In the below screenshot, on the left notice that the User State UI has loaded and on the right you can see it came from the new
cc-components/srcpackageIn the below recording, it shows how a toggle changes the theme of the user state component. Demo is shown in web component sample app but it works in both React and WC Samples:
Screen.Recording.2025-01-25.at.11.50.30.AM.mov
Pending Items
Integrating the momentum libSetup Dark/Light theme toggleSummary by CodeRabbit
New Features
Refactor
Documentation
Chores