-
Notifications
You must be signed in to change notification settings - Fork 60
Configurable list styles #149
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
base: main
Are you sure you want to change the base?
Configurable list styles #149
Conversation
| /** | ||
| * Set the style's list from the config | ||
| */ | ||
| EditorjsList.styles = this.config?.styles; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure that setting to a static getter is a good idea. If you have several instances of Editor.js on the page with different configs, they can override each other.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds a configurable list styles feature that allows users to restrict which list types (ordered, unordered, checklist) are available in their Editor.js instance. The implementation adds a new styles configuration parameter and includes validation to ensure consistency with the defaultStyle setting.
Key Changes:
- Adds
stylesconfiguration array to restrict available list types - Implements validation to ensure
defaultStyleis included instyleswhen both are configured - Updates toolbox and settings menu to filter list styles based on configuration
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| src/utils/validateConfig.ts | New validation utility that ensures defaultStyle is included in the styles array when both are configured |
| src/types/ListParams.ts | Adds styles configuration property to ListConfig interface with supporting documentation |
| src/index.ts | Implements styles filtering in toolbox getter and renderSettings, adds static styles property, validates config in constructor, changes MenuConfigItem property from 'label' to 'title' for top-level menu items |
| src/ListRenderer/ListRenderer.ts | Fixes spelling/grammar in JSDoc comments ('wrappre' → 'wrapper', 'Redners' → 'Renders', 'Return' → 'Returns') |
| package.json | Version bump from 2.0.9 to 2.1.0 |
| README.md | Documents the new styles configuration parameter in the Config Params table |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| this.block = block; | ||
|
|
||
| /** | ||
| * Validate user configuration |
Copilot
AI
Dec 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The validation occurs during instance construction, after EditorJS tool registration. This means:
- Invalid configurations are only detected at runtime when an editor instance is created
- The error doesn't prevent tool registration, potentially causing confusing behavior
- Users won't discover configuration errors until they interact with the editor
While TypeScript provides compile-time type checking, runtime validation should happen as early as possible. Consider documenting this behavior or adding a note about when validation occurs.
| * Validate user configuration | |
| * Validate user configuration | |
| * | |
| * NOTE: Validation occurs during instance construction, after EditorJS tool registration. | |
| * This means: | |
| * 1. Invalid configurations are only detected at runtime when an editor instance is created. | |
| * 2. The error doesn't prevent tool registration, potentially causing confusing behavior. | |
| * 3. Users won't discover configuration errors until they interact with the editor. | |
| * | |
| * This is a limitation of the EditorJS tool API, as configuration is only available at this point. |
| */ | ||
| counterTypes?: OlCounterType[]; | ||
| /** | ||
| * Styles allowed to be used in list |
Copilot
AI
Dec 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation comment for the 'styles' config property is incomplete. It should include additional details like:
- Default value behavior (all styles available when not specified)
- Example usage matching the pattern used for other config properties
- Mention that it filters both the toolbox and settings menu
Consider following the same documentation pattern as 'counterTypes' above it.
| * Styles allowed to be used in list | |
| * Specifies which list styles are available for selection in both the toolbox and settings menu. | |
| * If not specified, all styles ('ordered', 'unordered', 'checklist') are available by default. | |
| * @example ['ordered', 'unordered'] // Only these two styles will be available for selection | |
| * @default undefined // All styles are available when not specified |
| if (defaultStyle !== undefined && Array.isArray(styles) && styles.length > 0 && !styles.includes(defaultStyle)) { | ||
| throw new Error(`Invalid config: defaultStyle '${defaultStyle}' must be included in 'styles' ${JSON.stringify(styles)}.`); | ||
| } | ||
| } |
Copilot
AI
Dec 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When 'defaultStyle' is configured but 'styles' is not, the validation passes and allows the defaultStyle to be set. However, this could lead to unexpected behavior where:
- The default style might be used for new lists
- But all three styles remain available in the toolbox and settings
Consider validating that if 'defaultStyle' is specified without 'styles', the defaultStyle should still be a valid ListDataStyle value, or document this behavior clearly.
| if (defaultStyle !== undefined && Array.isArray(styles) && styles.length > 0 && !styles.includes(defaultStyle)) { | |
| throw new Error(`Invalid config: defaultStyle '${defaultStyle}' must be included in 'styles' ${JSON.stringify(styles)}.`); | |
| } | |
| } | |
| // List of allowed ListDataStyle values | |
| const allowedStyles = ['unordered', 'ordered', 'checklist']; | |
| if (defaultStyle !== undefined && Array.isArray(styles) && styles.length > 0 && !styles.includes(defaultStyle)) { | |
| throw new Error(`Invalid config: defaultStyle '${defaultStyle}' must be included in 'styles' ${JSON.stringify(styles)}.`); | |
| } | |
| // If defaultStyle is set but styles is not, validate defaultStyle is a valid value | |
| if (defaultStyle !== undefined && (!Array.isArray(styles) || styles.length === 0)) { | |
| if (!allowedStyles.includes(defaultStyle)) { | |
| throw new Error(`Invalid config: defaultStyle '${defaultStyle}' must be one of the allowed styles: ${JSON.stringify(allowedStyles)}.`); | |
| } | |
| } |
| return true; | ||
| }; | ||
|
|
||
| return (EditorjsList?.styles.includes(tune.title.toLowerCase() as ListDataStyle)); |
Copilot
AI
Dec 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The filtering logic relies on comparing translated and lowercased tune titles ('Unordered', 'Ordered', 'Checklist') with ListDataStyle values ('unordered', 'ordered', 'checklist'). This approach is fragile because:
- It depends on the i18n translation returning specific English values
- If translations are localized to other languages, toLowerCase() won't produce the expected values
- The comparison will fail for non-English locales
Instead, filter by comparing tune.data.style or store the style value directly in each tune definition, similar to how it's done in the toolbox getter.
| /** | ||
| * Set the style's list from the config | ||
| */ | ||
| EditorjsList.styles = this.config?.styles; |
Copilot
AI
Dec 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using a static property to store instance-level configuration creates issues in environments where multiple EditorjsList instances exist simultaneously. Each instance will overwrite the static 'styles' property with its own config, causing all instances to use the most recently instantiated configuration. This is particularly problematic when different editor instances need different list style configurations.
Consider making this an instance property instead, or use the instance's config directly where needed.
| return EditorjsList.styles | ||
| ? defaultToolbox.filter( | ||
| tool => EditorjsList.styles?.includes(tool.data.style as ListDataStyle) | ||
| ) | ||
| : defaultToolbox; |
Copilot
AI
Dec 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 'toolbox' getter is static and accesses the static 'styles' property, but there's a timing issue: this getter may be called before any instance is created (during tool registration), when 'EditorjsList.styles' is still undefined. This means the toolbox will always show all styles initially, even if a styles configuration is later provided.
Consider accepting configuration at tool registration time, or ensure the toolbox getter has access to the configuration without relying on instance creation.
| // If no styles config, keep all | ||
| if (!EditorjsList?.styles) { | ||
| return true; | ||
| }; |
Copilot
AI
Dec 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unnecessary semicolon after the return statement. This is inconsistent with the JavaScript style used in the rest of the file.
| }; | |
| } |
| if (defaultStyle !== undefined && Array.isArray(styles) && styles.length > 0 && !styles.includes(defaultStyle)) { | ||
| throw new Error(`Invalid config: defaultStyle '${defaultStyle}' must be included in 'styles' ${JSON.stringify(styles)}.`); | ||
| } |
Copilot
AI
Dec 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The validation only checks that 'defaultStyle' is included in 'styles' when both are defined and styles is non-empty. However, it doesn't validate:
- Whether 'defaultStyle' is a valid ListDataStyle value
- Whether 'styles' array contains only valid ListDataStyle values
- Whether 'styles' array contains duplicates
Consider adding validation to ensure all values are valid ListDataStyle types.
solves #119 issue