-
Notifications
You must be signed in to change notification settings - Fork 45
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
feat(cc-widgets): added new UI for call controls #405
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 Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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
Documentation and Community
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
|
||
import {CallControlPresentationalProps} from '../task.types'; | ||
import './call-control.styles.scss'; | ||
import {ButtonPill} from '@momentum-ui/react-collaboration'; | ||
import {Button, Icon} from '@momentum-design/components/dist/react'; |
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.
Why are we using the @momentum-design components for these? Shouldn't everything come from react-collaboration?
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.
We have ButtonCircle as well as Icon
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.
updated button. icon kept as it is as collaboration icons works properly. we can fix it once we fix i con issue
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.
Please check the comments
tooltip: isHeld ? 'Resume the call' : 'Hold the call', | ||
className: 'call-control-button', | ||
}, | ||
{ | ||
icon: isRecording ? 'record-paused-bold' : 'record-bold', | ||
onClick: () => handletoggleRecording(), | ||
tooltip: isRecording ? 'Pause Recording' : 'Resume Recording', | ||
className: 'call-control-button', | ||
}, | ||
{ | ||
icon: 'cancel-regular', | ||
onClick: endCall, | ||
tooltip: 'End call', |
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.
Can we add all the tooltip text and UI labels to a constant file?
<Text className="wrapup-header" tagName={'small'} type="subheader-primary"> | ||
Wrap-up Interaction | ||
</Text> |
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.
This one doesn't match the Figma Specs. In terms of positioning, size and weight. Please check.
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.
Updated
Wrap-up Interaction | ||
</Text> | ||
<div className="wrapup-select-wrapper"> | ||
<SelectNext |
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.
This select component height and width as well doesn't match with the Figma specs. Please check.
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.
Updated
className="wrapup-popover" | ||
> | ||
<Text className="wrapup-header" tagName={'small'} type="subheader-primary"> | ||
Wrap-up Interaction |
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.
Can we have this label in a constant file?
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.
We have many texts that are directly added in the HTML, is it okay that we have a ticket for i18n and we move everything as part of that?
items={wrapupCodes} | ||
showBorder={false} | ||
placeholder="Select" | ||
label="Wrap-up reason" |
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.
Let's add this label to constants as well
onPress={() => wrapupCall(selectedWrapupReason, selectedWrapupId)} | ||
disabled={selectedWrapupId && selectedWrapupReason ? false : true} | ||
> | ||
submit & Wrapup |
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.
submit & Wrapup | |
Submit & Wrap up |
Also ensure that this comes from the constant file
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.
Updated
import {ButtonPill} from '@momentum-ui/react-collaboration'; | ||
import {PopoverNext, SelectNext, TooltipNext, Text, ButtonCircle, ButtonPill} from '@momentum-ui/react-collaboration'; | ||
import {Item} from '@react-stately/collections'; | ||
import {Icon} from '@momentum-design/components/dist/react'; |
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.
Can't we import <IconNext/>
instead from the MRV2 itself for this?
CC - @Shreyas281299
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.
No we cant, the IconNext is not working. This component is the reason why SelectNext and other components with Icons are not working.
)} | ||
{wrapupRequired && ( | ||
<div className="wrapup-group"> | ||
<PopoverNext |
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.
Let's also check if the popover style matches the Figma. For instance, the border & padding in the Figma and the vidcast doesn't seem to match
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.
Updated
Screen.Recording.2025-03-06.at.6.40.04.PM.movTesting after my commit to fix UTs |
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.
Changes look good, Lets just update the UI according to the Figma as mentioned by Kesava
🎉 This PR is included in version 1.28.0-ccwidgets.29 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
# [1.28.0-ccwidgets.29](webex/widgets@v1.28.0-ccwidgets.28...v1.28.0-ccwidgets.29) (2025-03-06) ### Features * **cc-widgets:** added new UI for call controls ([webex#405](webex#405)) ([e48f48d](webex@e48f48d))
https://app.vidcast.io/share/2b6a8be0-f839-4e35-be73-a7f8006cf3f3