change tiptap #94
change tiptap #94
Conversation
This comment was marked as off-topic.
This comment was marked as off-topic.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
| onUpdate: ({ editor }) => { | ||
| if (typeof onUpdate !== 'function') return; | ||
|
|
||
| onUpdate(getContent(editor.getJSON()), editor.getText()); |
There was a problem hiding this comment.
Tiptapに書かれる内容は、
例えばtextで
a
b
c
d
ならhtmlでは
<p>a</p>
<p>b</p>
<p>c</p>
<p>d</p>
になる。
これをgetTextした場合は改行が余計に入るため、DBに保存するためには余計な改行をぬいてやる必要がある。一方でMarkdownに渡す時は、余計な改行を含めてそのまま渡してやらないと望んでいる形にならない。
one-pager-maker/src/pages/Edit.tsx
Outdated
| import {Tiptap} from "../components/tiptap.tsx"; | ||
|
|
||
| const toTiptapContent = (input: string): string => | ||
| input.split(/\n/).map(line => `<p>${line}</p>`).join(''); |
There was a problem hiding this comment.
DBに入れた内容は正規化されているので、TipTapに渡す時はむしろタグを足してやる必要がある
<p>a</p>
<p>b</p>
<p>c</p>
<p>d</p>
one-pager-maker/src/pages/Edit.tsx
Outdated
| const toTiptapContent = (input: string): string => | ||
| input.split(/\n/).map(line => `<p>${line}</p>`).join(''); | ||
|
|
||
| const toMarkdownText = (input: string): string => |
There was a problem hiding this comment.
DBに入れた内容は正規化されているので、Markdownに渡す時は余計な改行をむしろ足してやる必要がある
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- one-pager-maker/src/components/tiptap.tsx (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- one-pager-maker/src/components/tiptap.tsx
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (2)
one-pager-maker/src/components/tiptap.tsx (1)
12-38: TheTiptapcomponent is implemented correctly, utilizing various Tiptap extensions and handling updates robustly. Consider adding more details or a specific action plan for the TODO comment at line 29 to clarify what needs to be done.one-pager-maker/src/pages/Edit.tsx (1)
Line range hint
15-126: TheEditcomponent is well-implemented, integratingTiptapandContentConvertereffectively. Consider refactoring to improve separation of concerns, possibly by moving some of the state and effect logic into custom hooks or utility functions.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- one-pager-maker/src/components/tiptap.tsx (1 hunks)
- one-pager-maker/src/model/ContentConverter.ts (1 hunks)
- one-pager-maker/src/pages/Edit.tsx (5 hunks)
- one-pager-maker/tests/unit/ContentConverter.test.ts (1 hunks)
Additional context used
Biome
one-pager-maker/src/model/ContentConverter.ts
[error] 1-7: Avoid classes that contain only static members. (lint/complexity/noStaticOnlyClass)
Prefer using simple functions instead of classes with only static members.
Additional comments not posted (4)
one-pager-maker/src/model/ContentConverter.ts (2)
2-3: ThetoHtmlmethod implementation is straightforward and correctly transforms each line into a paragraph. Good use of regular expressions for line splitting.
5-6: ThetoMarkdownTextmethod correctly formats each line with double newlines, which is standard for Markdown to ensure proper paragraph separation.one-pager-maker/tests/unit/ContentConverter.test.ts (2)
4-8: The test case for thetoHtmlmethod is well-written and correctly checks the transformation of multiline strings into HTML paragraphs.
11-23: The test case for thetoMarkdownTextmethod accurately verifies that each line is separated by double newlines, adhering to Markdown standards.
| 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(''); | ||
| } |
There was a problem hiding this comment.
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.
| 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.
There was a problem hiding this comment.
質問 : 上の書き方はC#だと良く書く書き方です(classにもstaticをつけます)。使う側も、クラス名＋メソッド名の方がその内容をより端的に表せて分かりやすい気がします。下の書き方だとtoHtmlだけだと、何を変換するときに使うものかが分かりづらいような気がします。ただBiomeにnoStaticOnlyClassとあるところを見るとTypescriptでは下の書き方の方が推奨されるのでしょうか？
There was a problem hiding this comment.
@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.toHtmlやMarkdownConverter.toMarkdownTextとして使用することで、どの変換が行われるかが明確になり、コードの可読性が向上します。この方法で、名前空間を利用しながらも、クラスのオーバーヘッドを避けることができます。
There was a problem hiding this comment.
今教えてくれた、HtmlConverter.toHtmlとする書き方はtypescriptではポピュラーな書き方なのですか？
There was a problem hiding this comment.
@binnmti はい、TypeScriptでは、オブジェクトリテラルを使って関数をグループ化する方法は一定の人気がありますが、クラスやモジュールを使用する方法と比べると、それほど一般的ではありません。この方法は、特に関数が密接に関連しているがクラスインスタンスが必要でない場合に便利です。例えば、ユーティリティ関数や軽量な構造が必要な場合によく使用されます。
また、このパターンは、特定の機能をグループ化し、コードの読みやすさを向上させるために便利です。しかし、TypeScriptのコミュニティでは、より広範なソリューションとしてモジュールやクラスが推奨されることが多いです。そのため、プロジェクトの要件やチームの好みに応じて、適切な構造を選択することが重要です。
もし他の質問や懸念があれば、お気軽にお問い合わせください。
There was a problem hiding this comment.
この書き方ってパフォーマンス的に問題ないですか？色々聞いて、HtmlConvertertoHtmlという名前にするのが一番良い気がしています。
There was a problem hiding this comment.
@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.
現状の動き
One Pager Maker Edit
Cloud Firestore
\nという文字はそのままで、改行を表す情報は何も保存されていない。console.log
replace
\nを改行に置換してみる\\nを改行に置換してみる\n、\nという文字は\\nであるSummary by CodeRabbit
New Features
Bug Fixes
Chores
Tests
Style