-
Notifications
You must be signed in to change notification settings - Fork 3
SDKS-4670: Add support for extension in PhoneNumberCollector #573
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| '@forgerock/davinci-client': minor | ||
| --- | ||
|
|
||
| A new PhoneNumberExtensionCollector has been added to support phone number fields that include an extension. When a DaVinci PHONE_NUMBER field has showExtension: true, the SDK now produces a PhoneNumberExtensionCollector instead of a PhoneNumberCollector. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -95,3 +95,6 @@ GEMINI.md | |
| .claude/worktrees | ||
| .claude/settings.local.json | ||
| .opensource | ||
|
|
||
| # Polaris | ||
| .polaris-setup-progress.json | ||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -8,6 +8,9 @@ import type { | |||||
| DeviceAuthenticationCollector, | ||||||
| DeviceRegistrationCollector, | ||||||
| PhoneNumberCollector, | ||||||
| PhoneNumberExtensionCollector, | ||||||
| PhoneNumberExtensionInputValue, | ||||||
| PhoneNumberInputValue, | ||||||
| Updater, | ||||||
| } from '@forgerock/davinci-client/types'; | ||||||
|
|
||||||
|
|
@@ -19,11 +22,16 @@ import type { | |||||
| */ | ||||||
| export default function objectValueComponent( | ||||||
| formEl: HTMLFormElement, | ||||||
| collector: DeviceRegistrationCollector | DeviceAuthenticationCollector | PhoneNumberCollector, | ||||||
| collector: | ||||||
| | DeviceRegistrationCollector | ||||||
| | DeviceAuthenticationCollector | ||||||
| | PhoneNumberCollector | ||||||
| | PhoneNumberExtensionCollector, | ||||||
| updater: | ||||||
| | Updater<DeviceRegistrationCollector> | ||||||
| | Updater<DeviceAuthenticationCollector> | ||||||
| | Updater<PhoneNumberCollector>, | ||||||
| | Updater<PhoneNumberCollector> | ||||||
| | Updater<PhoneNumberExtensionCollector>, | ||||||
| submitForm: () => void, | ||||||
| ) { | ||||||
| if ( | ||||||
|
|
@@ -61,7 +69,7 @@ export default function objectValueComponent( | |||||
| buttonEl.textContent = option.label; | ||||||
| formEl.appendChild(buttonEl); | ||||||
| } | ||||||
| } else { | ||||||
| } else if (collector.type === 'PhoneNumberCollector') { | ||||||
| const phoneLabel = document.createElement('label'); | ||||||
| phoneLabel.textContent = collector.output.label || 'Phone Number'; | ||||||
| phoneLabel.className = 'object-options-title'; | ||||||
|
|
@@ -73,6 +81,9 @@ export default function objectValueComponent( | |||||
| phoneInput.setAttribute('name', 'phone-number-input'); | ||||||
| phoneInput.setAttribute('placeholder', 'Enter phone number'); | ||||||
|
|
||||||
| formEl.appendChild(phoneLabel); | ||||||
| formEl.appendChild(phoneInput); | ||||||
|
|
||||||
| // Add change event listener | ||||||
| phoneInput.addEventListener('change', (event) => { | ||||||
| // Properly type the event target | ||||||
|
|
@@ -84,13 +95,73 @@ export default function objectValueComponent( | |||||
| return; | ||||||
| } | ||||||
|
|
||||||
| updater({ | ||||||
| const phoneNumberInputValue: PhoneNumberInputValue = { | ||||||
| phoneNumber: selectedValue, | ||||||
| countryCode: collector.output.value?.countryCode || '', | ||||||
| } as any); | ||||||
| }; | ||||||
| const phoneNumberUpdater = updater as Updater<PhoneNumberCollector>; | ||||||
| phoneNumberUpdater(phoneNumberInputValue); | ||||||
| }); | ||||||
| } else if (collector.type === 'PhoneNumberExtensionCollector') { | ||||||
| const phoneLabel = document.createElement('label'); | ||||||
| phoneLabel.textContent = collector.output.label || 'Phone Number'; | ||||||
| phoneLabel.className = 'object-options-title'; | ||||||
| phoneLabel.setAttribute('for', 'phone-number-input-1'); | ||||||
|
|
||||||
| formEl.appendChild(phoneLabel); | ||||||
| formEl.appendChild(phoneInput); | ||||||
| const phoneInput = document.createElement('input'); | ||||||
| phoneInput.setAttribute('type', 'tel'); | ||||||
| phoneInput.setAttribute('id', 'phone-number-input-1'); | ||||||
| phoneInput.setAttribute('name', 'phone-number-input-1'); | ||||||
| phoneInput.setAttribute('placeholder', 'Enter phone number'); | ||||||
|
|
||||||
| const extensionLabel = document.createElement('label'); | ||||||
| extensionLabel.textContent = collector.output.extensionLabel || 'Extension'; | ||||||
| extensionLabel.className = 'object-options-title'; | ||||||
| extensionLabel.setAttribute('for', 'extension-input-1'); | ||||||
|
|
||||||
| const extensionInput = document.createElement('input'); | ||||||
| extensionInput.setAttribute('type', 'text'); | ||||||
| extensionInput.setAttribute('id', 'extension-input-1'); | ||||||
| extensionInput.setAttribute('name', 'extension-input-1'); | ||||||
| extensionInput.setAttribute('placeholder', 'Enter extension'); | ||||||
|
|
||||||
| const divEl = document.createElement('div'); | ||||||
| divEl.style = 'display: flex; gap: 8px;'; | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🌐 Web query:
💡 Result: In TypeScript's lib.dom.d.ts (as of 2024/2025 DOM updates), the ElementCSSInlineStyle interface declares the style property as readonly CSSStyleDeclaration. This means you cannot reassign the style property itself (e.g., element.style = someOtherDeclaration), but you can mutate the returned CSSStyleDeclaration object by setting individual properties (e.g., element.style.color = 'red'), using cssText (e.g., element.style.cssText = 'color: red'), or calling methods like setProperty. This design matches the DOM spec and browser behavior: the style property returns a live CSSStyleDeclaration object tied to the element's inline styles, which is mutable despite the readonly getter. Direct string assignment like element.style = 'color: red' works in browsers (forwarded to cssText), but TypeScript types it as readonly to prevent replacing the object, avoiding type errors for incompatible assignments. A long-open GitHub issue ( Citations:
🏁 Script executed: # Check if the file exists and read around line 129
file_path="e2e/davinci-app/components/object-value.ts"
if [ -f "$file_path" ]; then
echo "=== File exists ==="
# Show lines around 129 with context
sed -n '120,140p' "$file_path" | cat -n
else
echo "File not found: $file_path"
# Try to find similar files
find . -name "object-value.ts" -type f 2>/dev/null
fiRepository: ForgeRock/ping-javascript-sdk Length of output: 1080 🏁 Script executed: # Check for tsconfig files and TypeScript configuration
echo "=== Looking for tsconfig files ==="
find . -name "tsconfig*.json" -type f 2>/dev/null | head -20
echo ""
echo "=== Checking project structure around the file ==="
ls -la e2e/davinci-app/components/ 2>/dev/null | head -20Repository: ForgeRock/ping-javascript-sdk Length of output: 1851 🏁 Script executed: # Check the tsconfig files for davinci-app
echo "=== e2e/davinci-app/tsconfig.json ==="
cat e2e/davinci-app/tsconfig.json
echo ""
echo "=== e2e/davinci-app/tsconfig.app.json ==="
cat e2e/davinci-app/tsconfig.app.jsonRepository: ForgeRock/ping-javascript-sdk Length of output: 1186
The 🛠 Proposed fix- divEl.style = 'display: flex; gap: 8px;';
+ divEl.style.cssText = 'display: flex; gap: 8px;';📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||
| divEl.appendChild(phoneLabel); | ||||||
| divEl.appendChild(phoneInput); | ||||||
| divEl.appendChild(extensionLabel); | ||||||
| divEl.appendChild(extensionInput); | ||||||
|
|
||||||
| formEl.appendChild(divEl); | ||||||
|
|
||||||
| const phoneNumberExtensionUpdater = updater as Updater<PhoneNumberExtensionCollector>; | ||||||
|
|
||||||
| // Add change event listener for phone number input | ||||||
| phoneInput.addEventListener('change', (event) => { | ||||||
| const target = event.target as HTMLInputElement; | ||||||
| const phoneValue = target.value; | ||||||
| const extensionValue = extensionInput.value; | ||||||
| const phoneNumberExtensionInputValue: PhoneNumberExtensionInputValue = { | ||||||
| phoneNumber: phoneValue, | ||||||
| countryCode: collector.output.value?.countryCode || '', | ||||||
| extension: extensionValue, | ||||||
| }; | ||||||
|
|
||||||
| phoneNumberExtensionUpdater(phoneNumberExtensionInputValue); | ||||||
| }); | ||||||
|
|
||||||
| // Add change event listener for extension input | ||||||
| extensionInput.addEventListener('change', (event) => { | ||||||
| const target = event.target as HTMLInputElement; | ||||||
| const extensionValue = target.value; | ||||||
| const phoneValue = phoneInput.value; | ||||||
| const phoneNumberExtensionInputValue: PhoneNumberExtensionInputValue = { | ||||||
| phoneNumber: phoneValue, | ||||||
| countryCode: collector.output.value?.countryCode || '', | ||||||
| extension: extensionValue, | ||||||
| }; | ||||||
|
|
||||||
| phoneNumberExtensionUpdater(phoneNumberExtensionInputValue); | ||||||
| }); | ||||||
|
coderabbitai[bot] marked this conversation as resolved.
|
||||||
| } | ||||||
| } | ||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -31,7 +31,8 @@ test('Should render form fields', async ({ page }) => { | |||||||||||||||||
| await page.locator('#combobox-field-key-3').check(); | ||||||||||||||||||
| await page.locator('#combobox-field-key-2').uncheck(); | ||||||||||||||||||
|
|
||||||||||||||||||
| await page.locator('#phone-number-input').fill('1234567890'); | ||||||||||||||||||
| await page.locator('#phone-number-input-1').fill('1234567890'); | ||||||||||||||||||
| await page.locator('#extension-input-1').fill('7890'); | ||||||||||||||||||
|
|
||||||||||||||||||
| await expect(page.getByRole('button', { name: 'Flow Button' })).toBeVisible(); | ||||||||||||||||||
| await expect(page.getByRole('button', { name: 'Flow Link' })).toBeVisible(); | ||||||||||||||||||
|
|
@@ -42,9 +43,10 @@ test('Should render form fields', async ({ page }) => { | |||||||||||||||||
|
|
||||||||||||||||||
| await page.getByRole('button', { name: 'Submit' }).click(); | ||||||||||||||||||
| const request = await requestPromise; | ||||||||||||||||||
|
|
||||||||||||||||||
| const parsedData = JSON.parse(request.postData()); | ||||||||||||||||||
| const postData = request.postData(); | ||||||||||||||||||
| const parsedData = postData ? JSON.parse(postData) : {}; | ||||||||||||||||||
| const data = parsedData.parameters.data; | ||||||||||||||||||
|
|
||||||||||||||||||
|
Comment on lines
+46
to
+49
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Guard At Line 47, 🔧 Proposed fix const postData = request.postData();
-const parsedData = postData ? JSON.parse(postData) : {};
-const data = parsedData.parameters.data;
+expect(postData).toBeTruthy();
+const parsedData = JSON.parse(postData!);
+const data = parsedData?.parameters?.data;
+expect(data).toBeTruthy();📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||
| expect(data.actionKey).toBe('submit'); | ||||||||||||||||||
| expect(data.formData).toStrictEqual({ | ||||||||||||||||||
| 'text-input-key': 'The input', | ||||||||||||||||||
|
|
@@ -55,6 +57,7 @@ test('Should render form fields', async ({ page }) => { | |||||||||||||||||
| 'phone-field': { | ||||||||||||||||||
| phoneNumber: '1234567890', | ||||||||||||||||||
| countryCode: 'GB', | ||||||||||||||||||
| extension: '7890', // Tests PhoneNumberExtensionCollector | ||||||||||||||||||
| }, | ||||||||||||||||||
| }); | ||||||||||||||||||
| }); | ||||||||||||||||||
|
|
||||||||||||||||||
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.
Should we mark this as major if it is a breaking change?