@W-20975885 New launch agent location in header component#3606
@W-20975885 New launch agent location in header component#3606sf-tejas-nadkarni merged 8 commits intodevelopfrom
Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
175a224 to
742b72f
Compare
742b72f to
4c89336
Compare
feat: update the condition to avoid fab enable permission feat: update files
4c89336 to
2081fa1
Compare
There was a problem hiding this comment.
Looks good. @sf-tejas-nadkarni qq: will we be adding settings for customer, in the case they want to hide the agent on load and clicking on icon in header only opens it?
|
@sf-jhalak-maheshwari yes that will be a seperate feature with a new feature flag. |
c647115 to
c16ea45
Compare
6a4fc26 to
f5d1bc2
Compare
| const logout = useAuthHelper(AuthHelpers.Logout) | ||
| const navigate = useNavigation() | ||
| const storeLocatorEnabled = getConfig()?.app?.storeLocatorEnabled ?? STORE_LOCATOR_IS_ENABLED | ||
| const commerceAgentConfig = getCommerceAgentConfig() |
There was a problem hiding this comment.
It appears the code style is slightly inconsistent, most of the component access the global config object via getConfig().xxxxxx, and it seems shopper agent introduced its own utility getCommerceAgentConfig. I wonder if we want to be consistent, either create utilities for all other features (not just shopper agent), otherwise it looks inconsistent coding style
const storeLocatorEnabled = getConfig()?.app?.storeLocatorEnabled ?? STORE_LOCATOR_IS_ENABLED
const commerceAgentConfig = getCommerceAgentConfig()There was a problem hiding this comment.
Thats a good point, this is where they decided to change it into utility functions, #3230
| enableConversationContext: 'false', | ||
| conversationContext: [] | ||
| conversationContext: [], | ||
| enableAgentFromHeader: 'false' |
There was a problem hiding this comment.
any idea why the booleans are implemented as strings? I understand this is already there before your change but I'm curious to know if you happen to know the answer.
There was a problem hiding this comment.
Aggreed its cleaner to use boolean, I'm not sure why the API/Config design uses strings for flags.
I just opted for consistency
this is the official help doc Shopper Agent documentation.
| const onClient = typeof window !== 'undefined' | ||
|
|
||
| /** | ||
| * Launch the chat using the embedded service bootstrap API | ||
| * | ||
| * @function launchChat | ||
| * @returns {void} | ||
| */ | ||
| export function launchChat() { | ||
| if (!onClient) return | ||
|
|
||
| try { | ||
| // Launch chat using the embedded service bootstrap API | ||
| if ( | ||
| window.embeddedservice_bootstrap && | ||
| typeof window.embeddedservice_bootstrap.utilAPI.launchChat === 'function' | ||
| ) { | ||
| window.embeddedservice_bootstrap.utilAPI.launchChat() | ||
| } | ||
| } catch (error) { | ||
| console.error('Shopper Agent: Error launching chat', error) | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Open the shopper agent chat window | ||
| * | ||
| * Programmatically opens the embedded messaging widget by finding and clicking | ||
| * the embedded service chat button. This function can be called from custom | ||
| * UI elements like header buttons. | ||
| * | ||
| * @function openShopperAgent | ||
| * @returns {void} | ||
| */ | ||
| export function openShopperAgent() { | ||
| if (!onClient) return | ||
|
|
||
| try { | ||
| launchChat() | ||
| } catch (error) { | ||
| console.error('Shopper Agent: Error opening agent', error) | ||
| } | ||
| } |
There was a problem hiding this comment.
Have you considered to make this a React hook? There are many React hooks in this code base and it 's more consistent with the style and the technologies we used. something like below.
import { useCallback } from 'react'
declare global {
interface Window {
embeddedservice_bootstrap?: {
utilAPI: {
launchChat: () => void
}
}
}
}
/**
* Hook for launching the Salesforce Embedded Service chat
*/
export function useShopperAgent() {
const openChat = useCallback(() => {
if (typeof window === 'undefined') return
try {
window.embeddedservice_bootstrap?.utilAPI.launchChat()
} catch (error) {
console.error('Shopper Agent: Error launching chat', error)
}
}, [])
return { openChat }
}usage example
function ChatButton() {
const { openChat } = useShopperAgent()
return <button onClick={openChat}>Chat with us</button>
}
kevinxh
left a comment
There was a problem hiding this comment.
Overall looks good, left a few comments. thank you
| export function useOpenShopperAgent() { | ||
| const openShopperAgent = useCallback(() => { | ||
| launchChat() | ||
| }, []) | ||
|
|
||
| return openShopperAgent | ||
| } |
There was a problem hiding this comment.
I would encourage to update the function signature to make it more generic and return a collection of actions related to shopper agent rather than having "Open" in the hook's name and return a single function, this isn't extendable for future. Imagine we want to add a couple of more functionalities, with this design, we would have to either making a breaking change in terms of the function signature, or create more hooks like useActionOneShopperAgent , useActionTwoShopperAgent , useActionThreeShopperAgent etc.
export function useShopperAgent() {
const open = useCallback(() => {
launchChat()
}, [])
const actions = {
open,
// future proof and DRY the implementation
// this hook can be extended with new functionalities.
}
return actions
}Thoughts?
There was a problem hiding this comment.
Thats a good pracitce in react, i dont see it happening for this functionality but i can update it to make it simple
| export function launchChat() { | ||
| if (!onClient) return | ||
|
|
||
| try { | ||
| // Launch chat using the embedded service bootstrap API | ||
| if ( | ||
| window.embeddedservice_bootstrap?.utilAPI && | ||
| typeof window.embeddedservice_bootstrap.utilAPI.launchChat === 'function' | ||
| ) { | ||
| window.embeddedservice_bootstrap.utilAPI.launchChat() | ||
| } | ||
| } catch (error) { | ||
| console.error('Shopper Agent: Error launching chat', error) | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Open the shopper agent chat window | ||
| * | ||
| * Programmatically opens the embedded messaging widget by finding and clicking | ||
| * the embedded service chat button. This function can be called from custom | ||
| * UI elements like header buttons. | ||
| * | ||
| * @function openShopperAgent | ||
| * @returns {void} | ||
| */ | ||
| export function openShopperAgent() { | ||
| if (!onClient) return | ||
|
|
||
| try { | ||
| launchChat() | ||
| } catch (error) { | ||
| console.error('Shopper Agent: Error opening agent', error) | ||
| } | ||
| } |
There was a problem hiding this comment.
It doesn't look like we need to separate functions?
the only thing openShopperAgent does is to call launchChat, can we remove one of them and simplify the code?
There was a problem hiding this comment.
There are more feature coming in the next patch where we will need to add more conditions before launchChat thats why i have created it as seperate function
1f9334c to
0a1f5eb
Compare
| launchChat() | ||
| }, []) | ||
|
|
||
| return {actions: {open}} |
There was a problem hiding this comment.
I don't think you need the key "actions" because it's already an extensible object.
| return {actions: {open}} | |
| return { open } |
This way you can directly deconstruct it, usage example
const {open} = useShopperAgent()i'm just nitpicking.
There was a problem hiding this comment.
I'd prefer to keep the actions wrapper to add explicit namespace for executable functions. What you think?
| * @function openShopperAgent | ||
| * @returns {void} | ||
| */ | ||
| export function openShopperAgent() { |
There was a problem hiding this comment.
Do we need both launchChat and openShopperAgent function?
We can remove openShopperAgent and just have launchChat. Post that, renaming launchChat maybe a good option too
There was a problem hiding this comment.
openShopperAgent will have more conditions once we introduce flag to hide the FAB icon thats why added two seperate functions
| "defaultMessage": "View" | ||
| }, | ||
| "header.button.assistive_msg.ask_shopping_agent": { | ||
| "defaultMessage": "Ask Shopping Agent" |
There was a problem hiding this comment.
Do we want to add translations for other files as well or is there an automated process?
There was a problem hiding this comment.
translation command should have added more labels, let me look into that
Adds a configurable header button to launch the shopper agent, refactors shopper agent utilities for simplicity, and includes unit tests
Description
Header Component - Agent Button Integration
File: app/components/header/index.jsx
Description: Added conditional rendering of the SparkleIcon button in the header based on the enableLaunchAgentFromHeader configuration flag. The button triggers the shopper agent chat when clicked. Integrated with getCommerceAgentConfig utility for centralized configuration management.
Configuration Utility Enhancement
File: app/utils/config-utils.js
Description: Added getCommerceAgentConfig() utility function to centralize access to commerce agent configuration settings. Added enableLaunchAgentFromHeader to the default configuration options, providing a single source of truth for commerce agent settings across the application.
App Component - Agent Click Handler
File: app/components/_app/index.jsx
Description: Added onAgentClick handler that calls openShopperAgent() to programmatically open the shopper agent chat window when the header button is clicked.
Types of Changes
Changes
How to Test-Drive This PR
Header.Launch.Location.mov
Checklists
General
Accessibility Compliance
You must check off all items in one of the follow two lists:
or...
Localization