-
Notifications
You must be signed in to change notification settings - Fork 3
演習問題にSandpackによる実行環境を追加 #889
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?
Conversation
Deploying utcode-learn with
|
| Latest commit: |
9215bc0
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://488042e9.utcode-learn.pages.dev |
| Branch Preview URL: | https://code-sandpack.utcode-learn.pages.dev |
chvmvd
left a comment
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.
途中までですが
後は、Pull requestのタイトルと説明欄を書いていただけると、ありがたいです 🙇
Pull requestの大まかな内容と差分だけからではわからない意図などを書いていただけるとレビューがしやすくなり助かります 🥰
| <Sandpack | ||
| template="static" | ||
| files={{ | ||
| "/index.html": answerWhileHtml, | ||
| "/script.js": answerWhileJs, | ||
| }} | ||
| options={{ | ||
| activeFile: "/script.js", | ||
| }} | ||
| /> |
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.
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.
うーむ
これはどうしよう。
SyntaxError: Identifier 'i' has already been declared
となっている。
chvmvd
left a comment
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.
途中ですが、Sandpackを用いて実行環境を追加すること以外の差分が多く含まれているような気がします。たとえば、既存のViewSourceコンポーネントの削除や場所の移動などが挙げられます。一旦、Sandpackを用いて実行環境を追加することのみに絞っても良いかと思います。
| "/style.css": yellowHelloCssCss, | ||
| }} | ||
| options={{ | ||
| activeFile: "/index.html", |
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.
これはCSSに関する演習問題なので、CSSファイルをアクティブにする方が良いように思います。
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.
| "/style.css": fooCss, | ||
| }} | ||
| options={{ | ||
| activeFile: "/index.html", |
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.
同様
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.
| <ViewSource url={import.meta.url} path="_samples/answer-while" /> | ||
|
|
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.
なぜこちらは削除しているのでしょうか。
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.
復活しました
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.
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.
削除されています。
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.
復活しました
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.
削除されています。
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.
復活しました
|
@chvmvd |
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.
今回の変更に関係のない差分がまだまだありそうです 👀 またそれによって動かなくなっている箇所も散見されます。
GitHubではPull request内で提案された変更Files Changedタブで表示することができるため、その差分をよく確認してすべての差分が適切な差分となっていることを確認すると事故が防げて便利です。今回の場合はこちらから確認できます。
また、GitHubに限らずGitでソースコードを管理している場合はVS Codeを利用して差分を確認することができるため便利です。 cf. https://learn.utcode.net/docs/web-servers/git/#%E5%A4%89%E6%9B%B4%E5%B1%A5%E6%AD%B4%E3%82%92%E8%A1%A8%E7%A4%BA%E3%81%99%E3%82%8B
また、動作確認を行うとソフトウェアなどが正常に動作するかを未然に確認でき事故を防げます。ut.code(); LearnのリポジトリではCloudflareの機能を利用してPull requestで提案された内容のプレビューが自動で生成されるようになっています。今回の場合は、こちらから確認できます。また、ドキュメント内のコードについてもCodeSandboxの機能を用いてプレビューを確認できます。
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.
現状でstyle.cssに分かれていないため、index.htmlとstyle.cssに分離した。
| } | ||
| ``` | ||
|
|
||
| <ViewSource url={import.meta.url} path="_samples/max-no-else" /> |
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.
ut.code(); Learnではどうして、2つとの同じViewSourceなのに2つ置いてあるのか。
|
@chvmvd
|
|
Sandpackもpreviewで確認しました。 |
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.
ざっと見たところ、まだ少なくとも最低で2件は不要な差分が混じっていそうです 👀
|
7d017aa |
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.
軽く差分を見た感じ最低でもまだ3件はありますね👀
|
大量の |

変更内容
@codesandbox/sandpack-reactを追加raw-loaderを使ってサンプルファイルを読み込むように変更削除したファイル
Sandpack導入に伴い、ViewSource用に存在していた一部のサンプルファイルを削除