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

change tiptap #94

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Binary file modified one-pager-maker/bun.lockb
Binary file not shown.
4 changes: 4 additions & 0 deletions one-pager-maker/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,9 @@
"@reduxjs/toolkit": "^2.2.3",
"@tanstack/react-query": "^5.37.1",
"@testing-library/react": "^14.2.2",
"@tiptap/pm": "^2.4.0",
"@tiptap/react": "^2.4.0",
"@tiptap/starter-kit": "^2.4.0",
"class-variance-authority": "^0.7.0",
"clsx": "^2.1.1",
"firebase": "^10.8.0",
Expand All @@ -41,6 +44,7 @@
"result-type-ts": "^2.1.3",
"tailwind-merge": "^2.3.0",
"tailwindcss-animate": "^1.0.7",
"tiptap": "^1.32.2",
"tsyringe": "^4.8.0",
"zod": "^3.22.4"
},
Expand Down
38 changes: 38 additions & 0 deletions one-pager-maker/src/components/tiptap.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
import { useEditor, EditorContent } from '@tiptap/react'
import Document from '@tiptap/extension-document'
import Paragraph from '@tiptap/extension-paragraph'
import Text from '@tiptap/extension-text'
import HardBreak from '@tiptap/extension-hard-break'

interface MyEditorProps {
content: string;
onUpdate: (content: string, markdownText: string) => void;
}

export const Tiptap: React.FC<MyEditorProps> = ({ content, onUpdate }) => {
const editor = useEditor({
extensions: [
Document,
Paragraph,
Text,
HardBreak,
],
content: content,
editorProps: {
attributes: {
class: "p-1",
},
},
onUpdate: ({ editor }) => {
if (typeof onUpdate !== 'function') return;

// TODO: The same procedure is required if you copy
onUpdate(editor.getText({blockSeparator: '\n'}), editor.getText());
}
})
return (
<div>
<EditorContent editor={editor} />
</div>
)
}
12 changes: 0 additions & 12 deletions one-pager-maker/src/index.css
Original file line number Diff line number Diff line change
Expand Up @@ -6,18 +6,6 @@
@apply text-gray-900 leading-normal break-words;
}

.markdown > * + * {
@apply mt-0 mb-4;
}

.markdown li + li {
@apply mt-1;
}

.markdown li > p + p {
@apply mt-6;
}

.markdown strong {
@apply font-semibold;
}
Expand Down
7 changes: 7 additions & 0 deletions one-pager-maker/src/model/ContentConverter.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
export class ContentConverter {
static toHtml = (content: string): string =>
content.split(/\n/).map(line => `<p>${line}</p>`).join('');

static toMarkdownText = (content: string): string =>
content.split(/\n/).map(line => `${line}\n\n`).join('');
}
Comment on lines +1 to +7
Copy link

@coderabbitai coderabbitai bot Jun 12, 2024

Choose a reason for hiding this comment

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

Consider refactoring the ContentConverter class to use standalone functions instead of static methods, as the class only contains static members. This can simplify the structure and reduce overhead.

- export class ContentConverter {
-   static toHtml = (content: string): string => 
-       content.split(/\n/).map(line => `<p>${line}</p>`).join('');
- 
-   static toMarkdownText = (content: string): string =>
-       content.split(/\n/).map(line => `${line}\n\n`).join('');    
- }
+ export const toHtml = (content: string): string => 
+     content.split(/\n/).map(line => `<p>${line}</p>`).join('');
+ 
+ export const toMarkdownText = (content: string): string =>
+     content.split(/\n/).map(line => `${line}\n\n`).join('');
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
export class ContentConverter {
static toHtml = (content: string): string =>
content.split(/\n/).map(line => `<p>${line}</p>`).join('');
static toMarkdownText = (content: string): string =>
content.split(/\n/).map(line => `${line}\n\n`).join('');
}
export const toHtml = (content: string): string =>
content.split(/\n/).map(line => `<p>${line}</p>`).join('');
export const toMarkdownText = (content: string): string =>
content.split(/\n/).map(line => `${line}\n\n`).join('');
Tools
Biome

[error] 1-7: Avoid classes that contain only static members. (lint/complexity/noStaticOnlyClass)

Prefer using simple functions instead of classes with only static members.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

質問 : 上の書き方はC#だと良く書く書き方です(classにもstaticをつけます)。使う側も、クラス名+メソッド名の方がその内容をより端的に表せて分かりやすい気がします。下の書き方だとtoHtmlだけだと、何を変換するときに使うものかが分かりづらいような気がします。ただBiomeにnoStaticOnlyClassとあるところを見るとTypescriptでは下の書き方の方が推奨されるのでしょうか?

Copy link

Choose a reason for hiding this comment

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

@binnmti お問い合わせありがとうございます。TypeScriptでは、確かにC#のようにクラスで静的メソッドを使うこともできますが、インスタンスメソッドやプロパティが不要な場合、クラスのオーバーヘッドを避けるために単独の関数を使用することが推奨されています。BiomeのlintルールnoStaticOnlyClassはこの実践を推奨しています。

ただし、ご指摘のように、関数だけでは何を変換するかが分かりにくい場合、モジュールやオブジェクトリテラルを使って関数をグループ化することで、クラス名を使うような文脈を提供することができます。例えば、次のように書くことが可能です:

// ContentConverters.ts
export const HtmlConverter = {
    toHtml: (content: string): string => 
        content.split(/\n/).map(line => `<p>${line}</p>`).join('')
};

export const MarkdownConverter = {
    toMarkdownText: (content: string): string =>
        content.split(/\n/).map(line => `${line}\n\n`).join('')
};

このようにすると、HtmlConverter.toHtmlMarkdownConverter.toMarkdownTextとして使用することで、どの変換が行われるかが明確になり、コードの可読性が向上します。この方法で、名前空間を利用しながらも、クラスのオーバーヘッドを避けることができます。

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

今教えてくれた、HtmlConverter.toHtmlとする書き方はtypescriptではポピュラーな書き方なのですか?

Copy link

Choose a reason for hiding this comment

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

@binnmti はい、TypeScriptでは、オブジェクトリテラルを使って関数をグループ化する方法は一定の人気がありますが、クラスやモジュールを使用する方法と比べると、それほど一般的ではありません。この方法は、特に関数が密接に関連しているがクラスインスタンスが必要でない場合に便利です。例えば、ユーティリティ関数や軽量な構造が必要な場合によく使用されます。

また、このパターンは、特定の機能をグループ化し、コードの読みやすさを向上させるために便利です。しかし、TypeScriptのコミュニティでは、より広範なソリューションとしてモジュールやクラスが推奨されることが多いです。そのため、プロジェクトの要件やチームの好みに応じて、適切な構造を選択することが重要です。

もし他の質問や懸念があれば、お気軽にお問い合わせください。

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

この書き方ってパフォーマンス的に問題ないですか?色々聞いて、HtmlConvertertoHtmlという名前にするのが一番良い気がしています。

Copy link

Choose a reason for hiding this comment

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

@binnmti オブジェクトリテラルを使用して関数をグループ化する方法は、パフォーマンスにほとんど影響を与えません。TypeScriptやJavaScriptでは、オブジェクトリテラルは非常に軽量であり、クラスのインスタンス化に伴うオーバーヘッドが不要な場合には、特に有効です。このため、パフォーマンスの面から見ても問題はないと言えます。

また、HtmlConvertertoHtmlという名前についてですが、この名前は非常に明確で、何をする関数なのかをよく表しています。ただし、通常の命名規則に従うと、HtmlConverter.toHtmlのようにピリオドを使用してメソッドを示す方が一般的です。もしHtmlConvertertoHtmlという名前をクラスやオブジェクトなしで関数名として使用する場合は、convertHtmlToHtmlなどの動詞を含む形式がより適切かもしれません。これにより、関数が何をするのかがさらに明確になります。

ご検討のほど、よろしくお願いいたします。もし他にご質問があれば、お気軽にどうぞ。


Learnings added
User: binnmti
PR: Greek-Academy/one-pager-maker#94
File: one-pager-maker/src/model/ContentConverter.ts:1-7
Timestamp: 2024-06-13T22:48:42.172Z
Learning: User prefers clear and descriptive naming conventions for functions, expressing a preference for names like "HtmlConvertertoHtml" which explicitly describe the function's purpose.

Note: Learnings are effective only in the context of specific code segments. To apply general coding guidelines, please configure review instructions.

21 changes: 11 additions & 10 deletions one-pager-maker/src/pages/Edit.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ import {viewHistoryApi} from "@/api/viewHistoryApi.ts";
import {userApi} from "@/api/userApi.ts";
import {selectUser} from "@/redux/user/selector.ts";
import {useAppSelector} from "@/redux/hooks.ts";
import {Tiptap} from "../components/tiptap.tsx";
import {ContentConverter} from '../model/ContentConverter.ts'

function Edit() {
const {uid, documentId} = useParams<{ uid: string, documentId: string }>();
Expand All @@ -26,8 +28,8 @@ function Edit() {
const documentResult = documentApi.useGetDocumentQuery({ uid, documentId});
const document = documentResult.data?.value;
const [documentData, setDocumentData] = useState(document);
const [markdownText, setMarkdownText] = useState(ContentConverter.toMarkdownText(document?.contents ?? ''));
const updateDocument = documentApi.useUpdateDocumentMutation();

const editHistoryMutation = viewHistoryApi.useSetEditHistoryMutation();
const reviewHistoryMutation = viewHistoryApi.useSetReviewHistoryMutation();

Expand All @@ -40,7 +42,10 @@ function Edit() {
setDocumentData(prev => prev === undefined ? prev : ({ ...prev, [key]: val }))
}
const onChangeTitle = (e: React.ChangeEvent<HTMLInputElement>) => updateDocumentState("title", e.target.value);
const onChangeContents = (e: React.ChangeEvent<HTMLTextAreaElement>) => updateDocumentState("contents", e.target.value);
const onChangeContents = (content: string, markdownText: string) => {
updateDocumentState("contents", content);
setMarkdownText(markdownText);
}
binnmti marked this conversation as resolved.
Show resolved Hide resolved
const onChangeStatus = (e: React.ChangeEvent<HTMLSelectElement>) => updateDocumentState("status", e.target.value as Status);
const onChangeContributors = (e: React.ChangeEvent<HTMLInputElement>) => updateDocumentState("contributors", e.target.value.split(','));
const onChangeReviewers = (e: React.ChangeEvent<HTMLInputElement>) => updateDocumentState("reviewers", e.target.value.split(','));
Expand All @@ -61,7 +66,6 @@ function Edit() {

try {
const result = await updateDocument.mutateAsync({ uid, document: documentData });

// 更新したときに閲覧履歴を設定
const mutationArgs = {uid: userQuery.data.id, documentId: documentData.id, document: result };
if (documentData.status === 'reviewed') {
Expand Down Expand Up @@ -117,14 +121,11 @@ function Edit() {
</span>
</div>
<div className="flex p-1 w-full h-svh">
<textarea
className="border w-1/2 p-1"
value={documentData?.contents}
onChange={onChangeContents}
placeholder="Enter Markdown here"
/>
<div className="border w-1/2">
<Tiptap content={ContentConverter.toHtml(document?.contents ?? '')} onUpdate={onChangeContents} />
</div>
<div className="border w-1/2 p-1 overflow-scroll overflow-visible overflow-x-hidden">
<Markdown className='markdown'>{documentData?.contents}</Markdown>
<Markdown className='markdown'>{markdownText}</Markdown>
</div>
</div>
</div>
Expand Down
23 changes: 23 additions & 0 deletions one-pager-maker/tests/unit/ContentConverter.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
import { expect, test } from "vitest";
import {ContentConverter} from "../../src/model/ContentConverter";

test("Html", () => {
expect(ContentConverter.toHtml(`a
b
c
d`)).toEqual("<p>a</p><p>b</p><p>c</p><p>d</p>")});


test("MarkdownText", () => {
expect(ContentConverter.toMarkdownText(`a
b
c
d`)).toEqual(`a

b

c

d

`)});