Skip to content
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.

Sign up for GitHub

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

Fix interactive dialogs not showing #8427

Merged
merged 2 commits into from
Dec 18, 2024
Merged

Fix interactive dialogs not showing #8427

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
559e482
Fix interactive dialogs not showing
larkox Dec 18, 2024
de69aa9
Use conditionals
larkox Dec 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion app/components/settings/bool_setting.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import Label from './label';

type Props = {
label?: string;
value: boolean;
value?: boolean;
placeholder?: string;
helpText?: string;
errorText?: string;
Expand Down
2 changes: 1 addition & 1 deletion app/components/settings/radio_setting/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ type Props = {
onChange: (value: string) => void;
helpText?: string;
errorText?: string;
value: string;
value?: string;
testID: string;
}
function RadioSetting({
Expand Down
2 changes: 1 addition & 1 deletion app/components/settings/text_setting.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ type Props = {
maxLength?: number;
optional: boolean;
onChange: (value: string) => void;
value: string;
value: string | undefined;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: any reason why it's not

Suggested change
value: string | undefined;
value?: string;

to match the ones above?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First one I did, I forgot to update 😅

multiline: boolean;
keyboardType: KeyboardTypeOptions;
secureTextEntry: boolean;
Expand Down
29 changes: 22 additions & 7 deletions app/screens/interactive_dialog/dialog_element.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,21 @@ function selectKeyboardType(type: InteractiveDialogElementType, subtype?: Intera
return selectKB(subtype);
}

function getStringValue(value: string | number | boolean | string[] | undefined): string | undefined {
Copy link
Contributor

@rahimrahman rahimrahman Dec 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any value (heh, pun intended) in converting false to string and gets "false"?

I was thinking of option "true" / "false", but I have a feeling this gets converted somewhere already.

    if (typeof value === 'number' || typeof value === 'boolean') {
        return value.toString();
    }

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe if I understand correctly, the initValues function would have converted that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
function getStringValue(value: string | number | boolean | string[] | undefined): string | undefined {
function getStringValue(value?: string | number | boolean | string[]): string | undefined {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True/false values should only be used on Boolean settings, and those will use the getBooleanValue. If a boolean value gets into this point, probably is because something is "wrong", and undefined is the safe approach.

But I don't have a strong opinion on this. Nevertheless, it "should never happen".

Regarding the change to getStringValue, I would argue the argument is not optional. You must provide an argument, just that that argument can be undefined. 0/5 on that too, so if you have a strong opinion, happy to change it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking of consistency. Since if people can pass undefined, it's almost like not
passing anything. But it is silly to see getStringValue(). I rather see people send getStringValue(undefined) instead of getStringValue().

Long to short: don't change. :P

if (typeof value === 'string') {
return value;
}
if (typeof value === 'number') {
return value.toString();
}

return undefined;
}

function getBooleanValue(value: string | number | boolean | string[] | undefined): boolean | undefined {
return typeof value === 'boolean' ? value : undefined;
}

type Props = {
displayName: string;
name: string;
Expand All @@ -34,7 +49,7 @@ type Props = {
dataSource?: string;
optional?: boolean;
options?: PostActionOption[];
value: string|number|boolean|string[];
value: string|number|boolean|string[]|undefined;
rahimrahman marked this conversation as resolved.
Show resolved Hide resolved
onChange: (name: string, value: string|number|boolean|string[]) => void;
}
function DialogElement({
Expand All @@ -59,7 +74,7 @@ function DialogElement({
return;
}
onChange(name, newValue);
}, [onChange, type, subtype]);
}, [type, subtype, onChange, name]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for the parseInt above, does newValue as string work if newValue is a string[]?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea is that string[] is only getting into this function if we were to accept multiselect selectors (we accept that for Apps but not for Interactive Dialogs). But then the type and subtype won't be text and number.


const handleSelect = useCallback((newValue: DialogOption | undefined) => {
if (!newValue) {
Expand All @@ -68,7 +83,7 @@ function DialogElement({
}

onChange(name, newValue.value);
}, [onChange]);
}, [name, onChange]);

switch (type) {
case 'text':
Expand All @@ -77,7 +92,7 @@ function DialogElement({
<TextSetting
label={displayName}
maxLength={maxLength || (type === 'text' ? TEXT_DEFAULT_MAX_LENGTH : TEXTAREA_DEFAULT_MAX_LENGTH)}
value={value as string}
value={getStringValue(value)}
placeholder={placeholder}
helpText={helpText}
errorText={errorText}
Expand All @@ -102,7 +117,7 @@ function DialogElement({
errorText={errorText}
placeholder={placeholder}
showRequiredAsterisk={true}
selected={value as string}
selected={getStringValue(value)}
roundedBorders={false}
testID={testID}
/>
Expand All @@ -116,14 +131,14 @@ function DialogElement({
options={options}
onChange={handleChange}
testID={testID}
value={value as string}
value={getStringValue(value)}
/>
);
case 'bool':
return (
<BoolSetting
label={displayName}
value={value as boolean}
value={getBooleanValue(value)}
placeholder={placeholder}
helpText={helpText}
errorText={errorText}
Expand Down
9 changes: 3 additions & 6 deletions app/screens/interactive_dialog/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ function InteractiveDialog({
base.showAsAction = 'always';
base.color = theme.sidebarHeaderTextColor;
return base;
}, [intl, submitting, theme]);
}, [intl, submitLabel, submitting, theme.sidebarHeaderTextColor]);

useEffect(() => {
setButtons(componentId, {
Expand Down Expand Up @@ -190,7 +190,7 @@ function InteractiveDialog({
} else {
close();
}
}, [elements, values, intl, url, callbackId, state]);
}, [elements, url, callbackId, state, values, serverUrl, intl]);

useEffect(() => {
const unsubscribe = Navigation.events().registerComponentListener({
Expand Down Expand Up @@ -219,7 +219,7 @@ function InteractiveDialog({
return () => {
unsubscribe.remove();
};
}, [serverUrl, url, callbackId, state, handleSubmit, submitting]);
}, [serverUrl, url, callbackId, state, handleSubmit, submitting, componentId, notifyOnCancel]);

useAndroidHardwareBackHandler(componentId, close);

Expand All @@ -246,9 +246,6 @@ function InteractiveDialog({
}
{Boolean(elements) && elements.map((e) => {
const value = secureGetFromRecord(values, e.name);
if (value === undefined) {
return null;
}
return (
<DialogElement
key={'dialogelement' + e.name}
Expand Down