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

Add a new Settings page in the Client side section #48

Merged
merged 3 commits into from
Jan 8, 2025

Conversation

gremlation
Copy link
Contributor

The sample code provided on the Examples page uses a deprecated way of adding settings and is missing a lot of details.

The new page shows the new way of doing things and explains the different data types, options, and events.

@christian-byrne
Copy link
Contributor

New Settings API

// Register a new setting
app.registerExtension({
  name: 'TestExtension1',
  settings: [
    {
      id: 'TestSetting',
      name: 'Test Setting',
      type: 'text',
      defaultValue: 'Hello, world!'
    }
  ]
})

// Get the value of a setting
const value = app.extensionManager.setting.get('TestSetting')

// Set the value of a setting
app.extensionManager.setting.set('TestSetting', 'Hello, universe!')

@christian-byrne
Copy link
Contributor

Setting Type

export interface SettingParams extends FormItem {
  id: keyof Settings
  defaultValue: any | (() => any)
  onChange?: (newValue: any, oldValue?: any) => void
  // By default category is id.split('.'). However, changing id to assign
  // new category has poor backward compatibility. Use this field to overwrite
  // default category from id.
  // Note: Like id, category value need to be unique.
  category?: string[]
  experimental?: boolean
  deprecated?: boolean
  // Deprecated values are mapped to new values.
  migrateDeprecatedValue?: (value: any) => any
  // Version of the setting when it was added
  versionAdded?: string
  // Version of the setting when it was last modified
  versionModified?: string
}

/**
 * The base form item for rendering in a form.
 */
export interface FormItem {
  name: string
  type: SettingInputType | SettingCustomRenderer
  tooltip?: string
  attrs?: Record<string, any>
  options?: Array<string | SettingOption>
}

@christian-byrne
Copy link
Contributor

christian-byrne commented Dec 31, 2024

There's also colorpicker and file upload components.

You can bind properties to the components with attrs. There is full documentation on all the components in Primevue docs.

For example This is the default number:

app.registerExtension({
  name: 'TestExtension4',
  settings: [
    {
      id: 'TestSetting222222',
      category: ['Test2','d'],
      name: 'Test Settingd',
      type: 'number',
      defaultValue: 22,
    }
  ]
})

Selection_680

You can look at InputNumber documentation to find props you want to use and then apply them:

app.registerExtension({
  name: 'TestExtension4',
  settings: [
    {
      id: 'TestSetting222222',
      category: ['Test2','d'],
      name: 'Test Settingd',
      type: 'number',
      defaultValue: 22,
      attrs: {showButtons: true, prefix: '$'}
    }
  ]
})

Selection_679

Btw, your explanations and writing are very good on these docs.

@gremlation gremlation force-pushed the client-side-settings branch from 83aa907 to fad2d3f Compare January 2, 2025 14:28
@gremlation
Copy link
Contributor Author

Thanks for the information @christian-byrne! I added references to the relevant components and updated some of the information that was incorrect, so it explains how to support floating point numbers in number inputs for example.

I wasn't able to figure out how to add a color picker or file input - are these definitely supported for settings, or are these only supported as widgets inside nodes? Do you have a working example?

@gremlation gremlation force-pushed the client-side-settings branch 2 times, most recently from 338e1f2 to a1f5af8 Compare January 2, 2025 14:34
@christian-byrne
Copy link
Contributor

Nice work, it's looking good.

Re: color and file picker components, I forgot to mention that they were added in this PR which is not in the stable build of the frontend yet. While it's waiting to be added, you can test it by following these instructions

@gremlation
Copy link
Contributor Author

Ah, got it. Thanks, I'll follow up with another PR to add those docs when it's in stable then.

@christian-byrne
Copy link
Contributor

Sounds good. Were you able to try using app.extensionManager.setting.get instead of app.ui.settings.getSettingValue? The later is marked as deprecated.

@gremlation gremlation force-pushed the client-side-settings branch from a1f5af8 to 4f1ab06 Compare January 2, 2025 18:24
@gremlation
Copy link
Contributor Author

Sorry, I didn't see that the first time around. It was marked as deprecated five days ago, my working copy didn't have those changes in. I've updated it now.

@gremlation
Copy link
Contributor Author

I added documentation for the color and image upload setting types. I couldn't get the default value to work for the image upload. I tried an HTTPS URL and a data URL.

@christian-byrne
Copy link
Contributor

It looks good to me! I just did console test with all the examples and they all work. Just one missing comma here.

You could also consider documenting how the onChange callback is invoked immediately after registration. Someone on Disc specifically asked for it to be documented a while ago. Up to you!

The sample code provided on the Examples page uses a deprecated way of
adding settings and is missing a lot of details.

The new page shows the new way of doing things and explains the
different data types, options, and events.
@gremlation gremlation force-pushed the client-side-settings branch from fbc04c8 to 04403e5 Compare January 7, 2025 06:09
@gremlation
Copy link
Contributor Author

Thanks! Fixed the comma and added a note about the change event.

@robinjhuang
Copy link
Member

Thanks for adding this!

@robinjhuang robinjhuang merged commit af2b725 into Comfy-Org:main Jan 8, 2025
2 checks passed
@gremlation gremlation deleted the client-side-settings branch January 9, 2025 05:03
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.

3 participants