Skip to content

Created @scriptural/react package#221

Draft
abelpz wants to merge 57 commits into
mainfrom
scriptural-react
Draft

Created @scriptural/react package#221
abelpz wants to merge 57 commits into
mainfrom
scriptural-react

Conversation

@abelpz

@abelpz abelpz commented Dec 31, 2024

Copy link
Copy Markdown
Contributor

Created lexical editor package for supporting usj


This change is Reviewable

abelpz and others added 9 commits December 16, 2024 15:22
# Conflicts:
#	pnpm-lock.yaml
- update lock file
- fix USJ version from `usfm2Usj`
- revert unintentional changes
- move nodes menu CSS
- fix menu closing when typing (improve selection change detection)
- pass in `scrRef` to plugin
- get `contextMarker`
- add poetry quote marker overwrites
@codesandbox

codesandbox Bot commented Dec 31, 2024

Copy link
Copy Markdown

Review or Edit in CodeSandbox

Open the branch in Web EditorVS CodeInsiders

Open Preview

@nx-cloud

nx-cloud Bot commented Jan 7, 2025

Copy link
Copy Markdown

View your CI Pipeline Execution ↗ for commit dbb4255.

Command Status Duration Result
nx run-many -t test ❌ Failed 33s View ↗
nx run-many -t lint ✅ Succeeded 1s View ↗
nx run-many -t typecheck ✅ Succeeded 25s View ↗

☁️ Nx Cloud last updated this comment at 2025-01-13 15:50:54 UTC

@netlify

netlify Bot commented Jan 11, 2025

Copy link
Copy Markdown

Deploy Preview for quick-draft failed. Why did it fail? →

Name Link
🔨 Latest commit c799f4a
🔍 Latest deploy log https://app.netlify.com/projects/quick-draft/deploys/68e6f2fcd644e3000958cf4c

@irahopkinson irahopkinson left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'm publishing this partial review for now since GH is acting weird.

Comment thread package.json Outdated
Comment thread packages/utilities/vite.config.ts.timestamp-1736554817502-0ee5419a06655.mjs Outdated
Comment thread pnpm-lock.yaml

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This file was out of date when I checked out the branch. Run pnpm i to update this file.

Comment thread apps/scriptural/src/App.tsx
Comment thread apps/scriptural/src/App.tsx

@irahopkinson irahopkinson left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Next partial review (GH still acting weird - perhaps its is the large number of files)

Comment thread packages/scriptural-react/API.md Outdated
Comment thread packages/scriptural-react/CONTRIBUTING.md Outdated
Comment thread packages/scriptural-react/CONTRIBUTING.md Outdated
Comment thread packages/scriptural-react/CONTRIBUTING.md Outdated
Comment thread packages/scriptural-react/package.json
Comment thread packages/scriptural-react/package.json
Comment thread apps/scriptural/package.json

@irahopkinson irahopkinson left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Another partial review. I've run out of week now. I'll get back to this when I get back after next week. Sorry for the delay.

content?: UsjParaContent[];
"link-id"?: NonWhitespaceString;
"link-href"?: NonWhitespaceString;
[key: `x-${string}`]: string;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I thought x- prefix was for custom attributes and z prefix for custom markers, the link you provided doesn't provide an example for z- attributes. Could it be a mistake in the docs?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I checked with the head of the USX/J/FM committee and it should be both x- and z- for properties. He helped me understand more about this if you want to chat in more detail.

Comment thread packages/shared/converters/usj/core/usj.d.ts Outdated
Comment thread packages/shared/converters/usj/core/usj.d.ts
Comment thread packages/scriptural-react/src/plugins/ChapterVerseUpdatePlugin/index.tsx Outdated
Comment thread packages/scriptural-react/src/plugins/ChapterVerseUpdatePlugin/index.tsx Outdated
Comment thread packages/scriptural-react/src/plugins/MarkerWatcherPlugin/index.tsx
Comment thread packages/scriptural-react/vite.config.ts

@irahopkinson irahopkinson left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Finally getting back to this review. It would be good to talk about your USJ nodes and if we can combine our usage of nodes.

Comment thread .npmrc

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

If you delete this file I think we will have trouble publishing to NPM. Was this file causing some issue?

});
}

export type ScriptureReference = {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

You should now import ScriptureReference from "shared/utils/get-marker-action.model" to keep consistent across the repo.

root: ({ children }) => {
return {
type: "USJ",
version: "0.2.1",

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The latest is v3.1. See main for this update.

lastTextObject: null,
};

function $isSerializedTextNode(node: SerializedLexicalNode): node is SerializedTextNode {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This function name doesn't strictly need to start with '$' and probably shouldn't since you are dealing with serialized data (rather than an instantiated Node).

lastTextObject: null,
};

function $isSerializedTextNode(node: SerializedLexicalNode): node is SerializedTextNode {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

You should add | null | undefined to the node argument type. You could also simplify the last two checks to node?.type === "text".

} from "lexical";

export const createSerializedRootNode = (children: SerializedLexicalNode[]): SerializedRootNode => {
return {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

For both these functions I would just use the simplified arrow function return format, i.e. ... => { ...<someObjectContent>... };

export type SerializedBlockNode = SerializedScriptureElementNode;

export class BlockNode extends ScriptureElementNode {
constructor(attributes: Attributes = {}, props?: NodeProps, tag?: string, key?: NodeKey) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

You can remove this constructor since the super constructor does the same thing.

export type SerializedInlineNode = SerializedScriptureElementNode;

export class InlineNode extends ScriptureElementNode {
constructor(attributes: Attributes = {}, props?: NodeProps, tag?: string, key?: NodeKey) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

You can also remove this constructor since the super constructor does the same thing.

export type Attributes = { [key: string]: string };

export type SerializedUsfmElementNode = Spread<
export type SerializedScriptureElementNode = Spread<

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Are you sure this should be renamed? It now clashes with another type of that same name. I would revert the 2 changes to this file if its not intentional.

}
};

let nodeType: UsjNode["type"] | undefined = undefined;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

NIT: assigning to undefined isn't needed since declaring it makes it undefined.

abelpz added 14 commits April 23, 2025 21:49
…al app

- Introduced `AppReferenceHandler` to manage scripture references across editors.
- Enhanced `Editor` and `AppBar` components to utilize the new reference handler.
- Added `ScriptureNavigator` component for user-friendly navigation between scripture references.
- Integrated `ScrollToReferencePlugin` to automatically scroll to the current reference in the editor.
- Updated context and props to support scripture reference handling throughout the application.
- Added `findVerseInChapterContext` function to locate verses considering their chapter context.
- Implemented helper functions `isElementAfter` and `isElementBefore` to determine element positioning in the document.
- Updated `scrollToReference` to utilize the new verse-finding logic.
- Added `FindReplacePlugin` to provide find and replace capabilities.
- Introduced `FindReplaceDialog` component for user interface.
- Integrated `SearchButton` in `CustomToolbar` to toggle the find and replace dialog.
- Updated `Editor` and related components to support the new functionality.
- Enhanced context management for search and replace operations.
…ality

- Added `TriggerKeyButton` to `CustomToolbar` for setting trigger key combinations.
- Integrated `TriggerKeyDialog` for user interaction with trigger keys.
- Updated `CustomToolbar` to include `ScripturalNodesMenuPlugin` and `CustomMarkersToolbar`.
- Wrapped `CustomToolbar` in `MarkersMenuProvider` within the `Editor` component for improved context management.
- Refactored toolbar layout for better organization and usability.
…ural editor

- Introduced `CustomSaveButton` in `CustomToolbar` to replace the previous `SaveButton`, adding support for unsaved changes indication.
- Implemented `useUnsavedChanges` hook in the `Editor` component to manage unsaved changes and integrate with the `HistoryPlugin`.
- Updated `SaveButton` to include automatic change detection and visual feedback for unsaved changes.
- Enhanced documentation to reflect new features and usage examples for tracking unsaved changes.
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.

2 participants