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.

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

Application settings management #1396

Merged
merged 10 commits into from
Jun 27, 2024

Conversation

rohinp14
Copy link
Contributor

No description provided.

Copy link

linux-foundation-easycla bot commented Jun 20, 2024

CLA Not Signed

Copy link

netlify bot commented Jun 20, 2024

Deploy Preview for papaya-valkyrie-395400 canceled.

Name Link
🔨 Latest commit 1f09020
🔍 Latest deploy log https://app.netlify.com/sites/papaya-valkyrie-395400/deploys/667d497a257407000827c143

@@ -0,0 +1,12 @@
export type SettingsProperty <T extends string | number | boolean | object = string> = {
Copy link
Contributor

Choose a reason for hiding this comment

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

naming conventions - files that contains a UI component or a class should be named to reflect the component name exactly, so ApplicationSettingsPanel.tsx exports the component ApplicationSettingsPanel.
An associated css filr follows the same naming, so we would have ApplicationSettingsPanel.css

Files with other content begin with lowercase. I generally create a somenameTypes.ts file if I find I'm amassing a large number of TypeScript definitions which are obscuring the actual code ina. file, then I might create such a file to contain all the shared types across multiple files e.g layoutTypes.ts, listTypes.ts.

In this case, we only have a small set of Type definitions, I'd suggest we just keep them within the ApplicationSettingsPanel.tsx file.

import { SingleSelectionHandler } from "packages/vuu-ui-controls/src";

// Determine the box type to be displayed
function getFormControl(property: SettingsProperty, changeHandler, selectHandler) {
Copy link
Contributor

Choose a reason for hiding this comment

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

needs to accept the current value of the properyy, which will come from the applicationSettings prop

}

// Generates application settings form
const SettingsForm = ({ properties, changeHandler, selectHandler }: {properties :SettingsSchema}) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

splitting the component into two (ApplicationSettingsPanel/SettingsForm) is a good idea, they will likely grow in complexity.

The SettingsForm should go into its own file. Sometimes I host more than one UI component in the same file if they are really tightly coupled, but not often. If we might ever want to use the SettingsForm in its own right, for example (and I think we might) then it definately warrants a file of its own

}

// Generates application settings form
const SettingsForm = ({ properties, changeHandler, selectHandler }: {properties :SettingsSchema}) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

we want the SettingsForm itself to deal with the differences between the changeHandlers of various different form controls. I'd suggest the SettingsForm can take exacctly the same props as ApplicationSettingsPanel for now.

So, the SettingsForm could look something like this,

export const SettingsForm = ({applicationSettingsSchema, applicationSettings, onSettingChanged }) => {

const selectHandler = useCallback((evt, [selectedValue]) => {
const propertyName = getPropertyNameFromElement(evt.target)
onSettingChanged(propertyName, selectedValue)
),[])

const changeHandler = useCallback((evt) => {
const propertyName = getPropertyNameFromElement(evt.target)
 const value = getTypedValue(evt.target, propertyName, applicationSettingsSchema )
onSettingChanged(propertyName, value)
),[])
}

  return (
    <div>
      {applicationSettingsSchema.properties.map((property: string | number | boolean) => (
        <FormField data-field={property.name} key={property.name}>
          <FormFieldLabel>{property.label}</FormFieldLabel>
          {getFormControl(property, changeHandler, selectHandler)}
        </FormField>
      ))}
    </div>
  );

@heswell heswell marked this pull request as ready for review June 27, 2024 13:03
@heswell heswell merged commit 66333f0 into finos:main Jun 27, 2024
10 of 12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants