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 phase 2 #1408

Merged
merged 5 commits into from
Jul 2, 2024

Conversation

rohinp14
Copy link
Contributor

@rohinp14 rohinp14 commented Jul 1, 2024

No description provided.

Copy link

linux-foundation-easycla bot commented Jul 1, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

Copy link

netlify bot commented Jul 1, 2024

Deploy Preview for papaya-valkyrie-395400 canceled.

Name Link
🔨 Latest commit 8124232
🔍 Latest deploy log https://app.netlify.com/sites/papaya-valkyrie-395400/deploys/66839b728d167b0008cbc949

@rohinp14 rohinp14 requested a review from heswell July 2, 2024 06:08
@rohinp14 rohinp14 self-assigned this Jul 2, 2024
Copy link
Contributor

@heswell heswell left a comment

Choose a reason for hiding this comment

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

Good work, few minor points, I'm going to go ahead and merge.

console.log(
`Cannot change setting '${propertyName}'.\nDid you forget to declare an ApplicationProvider ?`
),
settings: {
applicationSettings: {
Copy link
Contributor

Choose a reason for hiding this comment

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

we have an instance of a concrete settings structure here, along with a concrete implementation of an applicationSettingsSchema.

I think maybe its ok to keep the themeMode, as we are using it in the SaltProvider, but the rest don't belong in the library code, they would be provided by client code - i.e in the application that is using the ApplicationContext Provider.

Even with themeMode, we probably shouldn't assume that an app will support light and dark.

Sometimes it makes sense to provide default value(s) for the context object managed by a Provider. I don't think it does here.

useCallback,
} from "react";
import { HTMLAttributes } from "react";
import { SettingsForm, SettingsSchema } from "./SettingsForm";
Copy link
Contributor

Choose a reason for hiding this comment

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

SettingsSchema can be removed - per the lint error below, its not being used.

return <Input value={value} />;
}
}
import { useApplicationSettings } from "../application-provider";

// Props for Panel
export interface ApplicatonSettingsPanelProps
Copy link
Contributor

Choose a reason for hiding this comment

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

because ApplicationSettingsPanel no longer implements any props of its own (just inherits from HTMLDivEl;ement), no need to specify a type for the props, just use HTMLAttributes<HTMLDivElement> directly as the type.

If you DID want to declare a type, you would use type rather than interface, so

export  type ApplicatonSettingsPanelProps = HTMLAttributes<HTMLDivElement>;

this is just declaring an alias that doesn't add any new props.

// import { ApplicatonSettingsPanelProps } from "./ApplicationSettingsPanel";

//Props for Form
export interface ApplicatonSettingsPanelProps
Copy link
Contributor

Choose a reason for hiding this comment

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

SettingsFormProps

SyntheticEvent,
useCallback,
} from "react";
// import { ApplicatonSettingsPanelProps } from "./ApplicationSettingsPanel";
Copy link
Contributor

Choose a reason for hiding this comment

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

remove commented out code

@heswell heswell marked this pull request as ready for review July 2, 2024 15:39
@heswell heswell merged commit 2550f36 into finos:main Jul 2, 2024
11 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