Skip to content

feat(settings): Refactor settings form into sections (resolves #215). #242

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

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
import React from "react";

import {
FormControl,
FormHelperText,
FormLabel,
Input,
Textarea,
} from "@mui/joy";


interface SettingsFormFieldProps {
configKey: string;
helperText: string | React.ReactNode;
initialValue: string | number;
label: string;
type: string;
}

/**
* Displays a form field for user input of configuration values.
*
* @param props
* @param props.configKey
* @param props.helperText
* @param props.initialValue
* @param props.label
* @param props.type
* @return
*/
const SettingsFormField = ({
configKey,
helperText,
initialValue,
label,
type,
}: SettingsFormFieldProps) => (
<FormControl>
<FormLabel>
{label}
</FormLabel>
{"number" === type ?
<Input
defaultValue={initialValue}
name={configKey}
type={"number"}/> :
<Textarea
defaultValue={initialValue}
name={configKey}/>}
Comment on lines +42 to +49
Copy link

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Consider supporting more input types.

Currently, the component only handles "number" type specifically and defaults to a textarea for all other types. In the future, you might want to support more HTML input types (text, email, password, etc.) for better form control and validation.

-       {"number" === type ?
-           <Input
-               defaultValue={initialValue}
-               name={configKey}
-               type={"number"}/> :
-           <Textarea
-               defaultValue={initialValue}
-               name={configKey}/>}
+       {(() => {
+           switch(type) {
+               case "number":
+                   return (
+                       <Input
+                           defaultValue={initialValue}
+                           name={configKey}
+                           type={"number"}/>
+                   );
+               case "textarea":
+                   return (
+                       <Textarea
+                           defaultValue={initialValue}
+                           name={configKey}/>
+                   );
+               default:
+                   return (
+                       <Input
+                           defaultValue={initialValue}
+                           name={configKey}
+                           type={type}/>
+                   );
+           }
+       })()}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
{"number" === type ?
<Input
defaultValue={initialValue}
name={configKey}
type={"number"}/> :
<Textarea
defaultValue={initialValue}
name={configKey}/>}
{(() => {
switch (type) {
case "number":
return (
<Input
defaultValue={initialValue}
name={configKey}
type={"number"}
/>
);
case "textarea":
return (
<Textarea
defaultValue={initialValue}
name={configKey}
/>
);
default:
return (
<Input
defaultValue={initialValue}
name={configKey}
type={type}
/>
);
}
})()}

<FormHelperText>
{helperText}
</FormHelperText>
</FormControl>
);


export type {SettingsFormFieldProps};
export default SettingsFormField;
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
.settings-form-field-sections-group-container {
overflow-y: auto;
display: flex;
flex-direction: column;
flex-grow: 1;
gap: 0.75rem;
}

.settings-form-field-sections-group {
gap: 0.5rem;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,112 @@
import React from "react";

import {
AccordionGroup,
Box,
Link,
} from "@mui/joy";

import {
CONFIG_KEY,
LOCAL_STORAGE_KEY,
} from "../../../../../typings/config";
import {getConfig} from "../../../../../utils/config";
import {SettingsFormFieldProps} from "./SettingsFormField";
import SettingsFormFieldsSection from "./SettingsFormFieldsSection";
import ThemeSwitchFormField from "./ThemeSwitchFormField";

import "./SettingsFormFieldSectionsGroup.css";


type SettingsFormFields = SettingsFormFieldProps | React.ReactNode;

/**
* Gets form fields information for user input of configuration values.
*
* @return A list of form fields information or React nodes.
*/
const getConfigFormFieldSections = (): Array<{
name: string;
fields: SettingsFormFields[];
}> => [
{
name: "Common",
fields: [
<ThemeSwitchFormField key={"settings-theme-switch"}/>,
{
configKey: LOCAL_STORAGE_KEY.PAGE_SIZE,
helperText: "Number of log messages to display per page.",
initialValue: getConfig(CONFIG_KEY.PAGE_SIZE),
label: "View: Page size",
type: "number",
},
],
},
{
name: "KV-Pairs IR / JSON",
fields: [
{
configKey: LOCAL_STORAGE_KEY.DECODER_OPTIONS_FORMAT_STRING,
helperText: (
<span>
Format string for formatting a JSON log event as plain text. See the
{" "}
<Link
href={"https://docs.yscope.com/yscope-log-viewer/main/user-guide/format-struct-logs-overview.html"}
level={"body-sm"}
rel={"noopener"}
target={"_blank"}
>
format string syntax docs
</Link>
{" "}
or leave this blank to display the entire log event.
</span>
),
initialValue: getConfig(CONFIG_KEY.DECODER_OPTIONS).formatString,
label: "Decoder: Format string",
type: "text",
},
{
configKey: LOCAL_STORAGE_KEY.DECODER_OPTIONS_LOG_LEVEL_KEY,
helperText: "Key to extract the log level from.",
initialValue: getConfig(CONFIG_KEY.DECODER_OPTIONS).logLevelKey,
label: "Decoder: Log level key",
type: "text",
},
{
configKey: LOCAL_STORAGE_KEY.DECODER_OPTIONS_TIMESTAMP_KEY,
helperText: "Key to extract the log timestamp from.",
initialValue: getConfig(CONFIG_KEY.DECODER_OPTIONS).timestampKey,
label: "Decoder: Timestamp key",
type: "text",
},
],
},
];

/**
* Displays a group of form fields for user input of configuration values.
*
* @return
*/
const SettingsFormFieldSectionsGroup = () => (
<Box className={"settings-form-field-sections-group-container"}>
<AccordionGroup
className={"settings-form-field-sections-group"}
size={"sm"}
>
{getConfigFormFieldSections().map(
({name, fields}) => (
<SettingsFormFieldsSection
fields={fields}
key={name}
name={name}/>
)
)}
</AccordionGroup>
</Box>
);

export type {SettingsFormFields};
export default SettingsFormFieldSectionsGroup;
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
.settings-form-field-section-summary {
font-weight: 700 !important;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
import React from "react";

import {
Accordion,
AccordionDetails,
AccordionSummary,
} from "@mui/joy";

import SettingsFormField, {SettingsFormFieldProps} from "./SettingsFormField";
import {SettingsFormFields} from "./SettingsFormFieldSectionsGroup";

import "./SettingsFormFieldsSection.css";


interface SettingsFormFieldsSectionProps {
name: string;
fields: SettingsFormFields[];
}

/**
* Displays a section of form fields for user input of configuration values.
*
* @param props
* @param props.fields A list of form fields information or React nodes.
* @param props.name
* @return
*/
const SettingsFormFieldsSection = ({
fields,
name,
}: SettingsFormFieldsSectionProps) => (
<Accordion defaultExpanded={true}>
<AccordionSummary
className={"settings-form-field-section-summary"}
variant={"soft"}
>
{name}
</AccordionSummary>
<AccordionDetails>
{fields.map(
(f, index) => (
React.isValidElement(f) ?
f :
<SettingsFormField
key={index}
{...(f as SettingsFormFieldProps)}/>
)
Comment on lines +40 to +47
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Avoid using array index as key.

Using the array index as a key can lead to performance issues and unexpected behavior when items are added, removed, or reordered. Consider using a more stable identifier.

{fields.map(
    (f, index) => (
        React.isValidElement(f) ?
            f :
            <SettingsFormField
-               key={index}
+               key={`field-${(f as SettingsFormFieldProps).configKey}`}
                {...(f as SettingsFormFieldProps)}/>
    )
)}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
{fields.map(
(f, index) => (
React.isValidElement(f) ?
f :
<SettingsFormField
key={index}
{...(f as SettingsFormFieldProps)}/>
)
{fields.map(
(f, index) => (
React.isValidElement(f) ?
f :
<SettingsFormField
key={`field-${(f as SettingsFormFieldProps).configKey}`}
{...(f as SettingsFormFieldProps)}/>
)
}

)}
</AccordionDetails>
</Accordion>
);


export default SettingsFormFieldsSection;
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,3 @@

height: 100%;
}

.settings-form-fields-container {
overflow-y: auto;
display: flex;
flex-direction: column;
flex-grow: 1;
gap: 0.75rem;
}
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,8 @@ import React, {
} from "react";

import {
Box,
Button,
Divider,
FormControl,
FormHelperText,
FormLabel,
Input,
Link,
Textarea,
} from "@mui/joy";

import {NotificationContext} from "../../../../../contexts/NotificationContextProvider";
Expand All @@ -29,67 +22,13 @@ import {
TAB_NAME,
} from "../../../../../typings/tab";
import {ACTION_NAME} from "../../../../../utils/actions";
import {
getConfig,
setConfig,
} from "../../../../../utils/config";
import {setConfig} from "../../../../../utils/config";
import CustomTabPanel from "../CustomTabPanel";
import ThemeSwitchFormField from "./ThemeSwitchFormField";
import SettingsFormFieldSectionsGroup from "./SettingsFormFieldSectionsGroup";

import "./index.css";


/**
* Gets form fields information for user input of configuration values.
*
* @return A list of form fields information.
*/
const getConfigFormFields = () => [
{
helperText: (
<span>
[JSON] Format string for formatting a JSON log event as plain text. See the
{" "}
<Link
href={"https://docs.yscope.com/yscope-log-viewer/main/user-guide/format-struct-logs-overview.html"}
level={"body-sm"}
rel={"noopener"}
target={"_blank"}
>
format string syntax docs
</Link>
{" "}
or leave this blank to display the entire log event.
</span>
),
initialValue: getConfig(CONFIG_KEY.DECODER_OPTIONS).formatString,
key: LOCAL_STORAGE_KEY.DECODER_OPTIONS_FORMAT_STRING,
label: "Decoder: Format string",
type: "text",
},
{
helperText: "[JSON] Key to extract the log level from.",
initialValue: getConfig(CONFIG_KEY.DECODER_OPTIONS).logLevelKey,
key: LOCAL_STORAGE_KEY.DECODER_OPTIONS_LOG_LEVEL_KEY,
label: "Decoder: Log level key",
type: "text",
},
{
helperText: "[JSON] Key to extract the log timestamp from.",
initialValue: getConfig(CONFIG_KEY.DECODER_OPTIONS).timestampKey,
key: LOCAL_STORAGE_KEY.DECODER_OPTIONS_TIMESTAMP_KEY,
label: "Decoder: Timestamp key",
type: "text",
},
{
helperText: "Number of log messages to display per page.",
initialValue: getConfig(CONFIG_KEY.PAGE_SIZE),
key: LOCAL_STORAGE_KEY.PAGE_SIZE,
label: "View: Page size",
type: "number",
},
];

/**
* Handles the reset event for the configuration form.
*
Expand Down Expand Up @@ -156,27 +95,7 @@ const SettingsTabPanel = () => {
onReset={handleConfigFormReset}
onSubmit={handleConfigFormSubmit}
>
<Box className={"settings-form-fields-container"}>
<ThemeSwitchFormField/>
{getConfigFormFields().map((field, index) => (
<FormControl key={index}>
<FormLabel>
{field.label}
</FormLabel>
{"number" === field.type ?
<Input
defaultValue={field.initialValue}
name={field.key}
type={"number"}/> :
<Textarea
defaultValue={field.initialValue}
name={field.key}/>}
<FormHelperText>
{field.helperText}
</FormHelperText>
</FormControl>
))}
</Box>
<SettingsFormFieldSectionsGroup/>
<Divider/>
<Button
color={"primary"}
Expand Down