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

feat: Revive attachment-refs with remark #7597

Merged
merged 24 commits into from Apr 26, 2023

Conversation

arafubeatbox
Copy link
Contributor

@arafubeatbox arafubeatbox commented Apr 22, 2023

review task

https://redmine.weseek.co.jp/issues/120918

概要

plugin-attachment-refs を以下のように実装し直し

  • 名称を remark-attachment-refs に変更
  • vite を使うように変更(turborepo の設定も変更)
  • ts で記述するように変更
  • option parse ロジックを renderer service に移動し、react-markdown に受け渡し
  • ref, refs, refimg, refsimg, gallery それぞれの tag に対応するコンポーネントとして Ref, Refs, Refimg, Refsimg, Gallery を実装し、react-markdown に受け渡し

備考

@arafubeatbox arafubeatbox changed the title Feat/112904 120355 enable attachment refs feat/112904 120355 enable attachment refs Apr 22, 2023
@arafubeatbox arafubeatbox temporarily deployed to VRT April 22, 2023 03:38 — with GitHub Actions Inactive
@arafubeatbox arafubeatbox changed the title feat/112904 120355 enable attachment refs feat: 112904 120355 enable attachment refs Apr 22, 2023
* 1. when 'fileFormat' is image, render Attachment as an image
* 2. when 'fileFormat' is not image, render Attachment as an Attachment component
*/
// TODO: implement image carousel modal without using react-images
Copy link
Contributor Author

Choose a reason for hiding this comment

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

react-images が今は開発されておらず、今の環境だと動かなくなってしまいました。
react-images の README では
https://www.npmjs.com/package/react-responsive-carousel
がお勧めされていましたが、react-images と異なり modal ではないので少し追加実装が必要そうなのと、react-responsive-carousel もメンテナンスがされていないため、自前実装という方針もあると思っています。

そもそも carousel が必要かどうかの検討も必要かもですが、carousel の実装は、別ストーリーに切り出した方が良いと思っています。

Copy link
Contributor

Choose a reason for hiding this comment

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

@yuki-takei ご判断お願いします。

Copy link
Member

Choose a reason for hiding this comment

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

この PR からはスコープは外して別ストーリーでよいです。

イメージを小さく表示すると carousel ほしいなあって思うはず…

Copy link
Contributor Author

Choose a reason for hiding this comment

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

了解です。
https://redmine.weseek.co.jp/issues/121095
ストーリー作成し、README の carousel の説明部分をコメントアウトしました。

@arafubeatbox arafubeatbox temporarily deployed to VRT April 23, 2023 16:19 — with GitHub Actions Inactive
Copy link
Contributor

@jam411 jam411 Apr 24, 2023

Choose a reason for hiding this comment

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

検討の結果もし Image Carousel 廃止する場合は README の更新もお願いします。

Copy link
Contributor

@jam411 jam411 Apr 24, 2023

Choose a reason for hiding this comment

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

GROWI 内のコードには、IAttachment & HasObjectId と書かれている箇所があるから、IAttachmentHasIdを使用するように改修します。
ただし、それは別のストーリーでほかの誰かにやってもらいます。

Comment on lines 28 to 35
let message;

if (refsContext.options?.prefix != null) {
message = `${refsContext.options.prefix} and descendant pages have no attachments`;
}
else {
message = `${refsContext.pagePath} has no attachments`;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

条件演算子を使ってもいいと思いました。例えば、

const message = refsContext.options?.prefix != null ? `${refsContext.options.prefix} and descendant pages have no attachments` : `${refsContext.pagePath} has no attachments`

* 1. when 'fileFormat' is image, render Attachment as an image
* 2. when 'fileFormat' is not image, render Attachment as an Attachment component
*/
// TODO: implement image carousel modal without using react-images
Copy link
Contributor

Choose a reason for hiding this comment

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

@yuki-takei ご判断お願いします。

@arafubeatbox arafubeatbox temporarily deployed to VRT April 24, 2023 11:44 — with GitHub Actions Inactive
@arafubeatbox arafubeatbox temporarily deployed to VRT April 24, 2023 12:02 — with GitHub Actions Inactive
@arafubeatbox arafubeatbox changed the title feat: 112904 120355 enable attachment refs feat: enable attachment refs Apr 24, 2023
apps/app/src/client/services/renderer/renderer.tsx Outdated Show resolved Hide resolved
* 1. when 'fileFormat' is image, render Attachment as an image
* 2. when 'fileFormat' is not image, render Attachment as an Attachment component
*/
// TODO: implement image carousel modal without using react-images
Copy link
Member

Choose a reason for hiding this comment

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

この PR からはスコープは外して別ストーリーでよいです。

イメージを小さく表示すると carousel ほしいなあって思うはず…

turbo.json Outdated Show resolved Hide resolved
turbo.json Outdated Show resolved Hide resolved
turbo.json Outdated Show resolved Hide resolved
@arafubeatbox arafubeatbox temporarily deployed to VRT April 25, 2023 15:23 — with GitHub Actions Inactive
@reg-suit
Copy link

reg-suit bot commented Apr 25, 2023

reg-suit detected visual differences.

Check this report, and review them.

🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴🔴

🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵🔵

What do the circles mean? The number of circles represent the number of changed images.
🔴 : Changed items, ⚪ : New items, ⚫ : Deleted items, and 🔵 Passed items

How can I change the check status? If reviewers approve this PR, the reg context status will be green automatically.

@yuki-takei yuki-takei changed the title feat: enable attachment refs feat: Revive attachment-refs with remark Apr 26, 2023
@yuki-takei yuki-takei merged commit de5dce8 into master Apr 26, 2023
28 checks passed
@yuki-takei yuki-takei deleted the feat/112904-120355-enable-attachment-refs branch April 26, 2023 09:38
@github-actions github-actions bot mentioned this pull request Apr 26, 2023
@yuki-takei yuki-takei mentioned this pull request May 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

3 participants