Skip to content

Fix term tag#407

Merged
aster-void merged 8 commits into
masterfrom
fix-term-tag
Oct 5, 2023
Merged

Fix term tag#407
aster-void merged 8 commits into
masterfrom
fix-term-tag

Conversation

@aster-void
Copy link
Copy Markdown
Contributor

No description provided.

@cloudflare-workers-and-pages
Copy link
Copy Markdown

cloudflare-workers-and-pages Bot commented Sep 30, 2023

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 540ea73
Status: ✅  Deploy successful!
Preview URL: https://1495f77c.utcode-learn.pages.dev
Branch Preview URL: https://fix-term-tag.utcode-learn.pages.dev

View logs

Copy link
Copy Markdown
Contributor

@chvmvd chvmvd left a comment

Choose a reason for hiding this comment

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

definition.jsの差分は今回関係ないので抜きたいのと、Prettierを書けてほしいです。

Copy link
Copy Markdown
Contributor

@chvmvd chvmvd left a comment

Choose a reason for hiding this comment

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

よいですねえ。

Comment thread src/components/Term/index.jsx
Comment thread src/components/Term/index.jsx Outdated
// referencePageTitleがundefinedならばエラーを投げる (明らかに人的ミスのため)
if (referencePageTitle === undefined) {
throw new Error(
`The page title of the reference " ${term.referencePage} " \n is not defined in \n[ src/components/Term/definition.js/, referencePageTitles ]`,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
`The page title of the reference " ${term.referencePage} " \n is not defined in \n[ src/components/Term/definition.js/, referencePageTitles ]`,
`The title for the reference page "${term.referencePage}" is not defined in the "referencePageTitles" object within the file "src/components/Term/definition.js".`,

Copy link
Copy Markdown
Contributor

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.

改行も[]もあったほうが読みやすいかなあ、と。

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

改行は、こちらで指定するものではないような気がします。br要素の使い方と同じ感じで。
[]に関しては、この[]の用法がいまいちよくわからなかったです。この[]にどのような意味があるのかが、わからなかった感じです。

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.

[]は消しておきます。改行は、ユーザーが見るものではないのでリンクと文章の分離ができればokだと思ってます。

Copy link
Copy Markdown
Contributor

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

@chvmvd chvmvd left a comment

Choose a reason for hiding this comment

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

LGTM!

@aster-void
Copy link
Copy Markdown
Contributor Author

マージします。

@aster-void aster-void merged commit fd225cd into master Oct 5, 2023
@aster-void aster-void deleted the fix-term-tag branch October 5, 2023 00:52
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