-
Notifications
You must be signed in to change notification settings - Fork 90
feat: enable headings to be configurable #754
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?
Changes from 6 commits
57310e4
9c4db30
1c1349d
b224f00
ca39bff
5c60067
b370be1
9478bbb
1ee1a03
ed93022
ca8bff7
cba70bf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -152,27 +152,35 @@ export class CommandDispatcher { | |
| this.editor.focus() | ||
| } | ||
|
|
||
| get #configuredHeadings() { | ||
| const configured = this.editorElement.config.get("headings") | ||
| const headings = Array.isArray(configured) ? configured : [ "h2", "h3", "h4" ] | ||
| return headings.filter((heading) => /^h[1-6]$/.test(heading)) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I feel like this is duplcating the default value. Mis-configuration producing errors is ok I think.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah - this is the one aspect having automated code review kind of misses and adds junk. Has you defensively programming around ghost scenarios that will likely never happen. Thanks for your thinking here |
||
| } | ||
|
|
||
| dispatchRotateHeadingFormat() { | ||
| const selection = $getSelection() | ||
| if (!$isRangeSelection(selection)) return | ||
|
|
||
| const headings = this.#configuredHeadings | ||
| if (headings.length === 0) return | ||
|
ShugKnight24 marked this conversation as resolved.
Outdated
|
||
|
|
||
| if ($isRootOrShadowRoot(selection.anchor.getNode())) { | ||
| selection.insertNodes([ $createHeadingNode("h2") ]) | ||
| selection.insertNodes([ $createHeadingNode(headings[0]) ]) | ||
| return | ||
| } | ||
|
|
||
| const topLevelElement = selection.anchor.getNode().getTopLevelElementOrThrow() | ||
|
ShugKnight24 marked this conversation as resolved.
Outdated
|
||
| let nextTag = "h2" | ||
| let nextTag = headings[0] | ||
| if ($isHeadingNode(topLevelElement)) { | ||
| const currentTag = topLevelElement.getTag() | ||
| if (currentTag === "h2") { | ||
| nextTag = "h3" | ||
| } else if (currentTag === "h3") { | ||
| nextTag = "h4" | ||
| } else if (currentTag === "h4") { | ||
| const currentIndex = headings.indexOf(currentTag) | ||
| if (currentIndex >= 0 && currentIndex < headings.length - 1) { | ||
| nextTag = headings[currentIndex + 1] | ||
| } else if (currentIndex === headings.length - 1) { | ||
| nextTag = null | ||
| } else { | ||
| nextTag = "h2" | ||
| nextTag = headings[0] | ||
| } | ||
|
ShugKnight24 marked this conversation as resolved.
Outdated
|
||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -288,10 +288,10 @@ export class LexicalToolbarElement extends HTMLElement { | |
| } | ||
|
|
||
| #closeDropdowns() { | ||
| this.#dropdowns.forEach((details) => { | ||
| details.open = false | ||
| }) | ||
| } | ||
| this.#dropdowns.forEach((details) => { | ||
| details.open = false | ||
| }) | ||
| } | ||
|
Comment on lines
+309
to
+312
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this indentation change looks correct |
||
|
|
||
| get #dropdowns() { | ||
| return this.querySelectorAll("details") | ||
|
|
||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I ❤️ the use of presets here
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🙏🏻 - Thank copilot. I originally had each in the various tests and it pointed out I could have a preset based on the codebase |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,63 @@ | ||
| import { expect, test } from "vitest" | ||
| import { createElement } from "../helpers/dom_helper" | ||
| import EditorConfiguration from "../../../src/editor/configuration" | ||
| import { configure } from "../../../src/index" | ||
|
|
||
| configure({ | ||
| default: { | ||
| headings: ["h2", "h3", "h4"] | ||
| }, | ||
| blog: { | ||
| headings: ["h2", "h3", "h4"], | ||
| }, | ||
|
ShugKnight24 marked this conversation as resolved.
Outdated
|
||
| minimal: { | ||
| headings: ["h2"], | ||
| }, | ||
| noHeadings: { | ||
| headings: [], | ||
| }, | ||
| }) | ||
|
|
||
| test("uses default headings", () => { | ||
| const element = createElement("<lexxy-editor></lexxy-editor>") | ||
| const config = new EditorConfiguration(element) | ||
| expect(config.get("headings")).toEqual(["h2", "h3", "h4"]) | ||
| }) | ||
|
|
||
| test("overrides headings with attribute", () => { | ||
| const element = createElement( | ||
| '<lexxy-editor headings=\'["h1", "h2", "h3", "h4", "h5", "h6"]\'></lexxy-editor>' | ||
| ) | ||
| const config = new EditorConfiguration(element) | ||
| expect(config.get("headings")).toEqual(["h1", "h2", "h3", "h4", "h5", "h6"]) | ||
| }) | ||
|
|
||
| test("overrides headings with attribute to include h1 and h5", () => { | ||
| const element = createElement( | ||
| '<lexxy-editor headings=\'["h1", "h2", "h5"]\'></lexxy-editor>' | ||
| ) | ||
| const config = new EditorConfiguration(element) | ||
| expect(config.get("headings")).toEqual(["h1", "h2", "h5"]) | ||
| }) | ||
|
|
||
| test("overrides headings with preset", () => { | ||
| const element = createElement("<lexxy-editor preset='blog'></lexxy-editor>") | ||
| const config = new EditorConfiguration(element) | ||
| expect(config.get("headings")).toEqual(["h2", "h3", "h4"]) | ||
| }) | ||
|
|
||
| test("restricts headings to a subset", () => { | ||
| const element = createElement( | ||
| "<lexxy-editor preset='minimal'></lexxy-editor>" | ||
| ) | ||
| const config = new EditorConfiguration(element) | ||
| expect(config.get("headings")).toEqual(["h2"]) | ||
| }) | ||
|
|
||
| test("handles empty headings array", () => { | ||
| const element = createElement( | ||
| "<lexxy-editor preset='noHeadings'></lexxy-editor>" | ||
| ) | ||
| const config = new EditorConfiguration(element) | ||
| expect(config.get("headings")).toEqual([]) | ||
| }) | ||
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.
PR description says the defaults were changed to H1–H6, but the code sets the default preset headings to
["h2", "h3", "h4"]. Either update the description to match the implementation, or adjust the defaultheadingspreset here (and any other defaults) to reflect the intended behavior.