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

fix: Drawio not available with GROWI slides #8725

Merged
merged 44 commits into from
May 13, 2024

Conversation

reiji-h
Copy link
Contributor

@reiji-h reiji-h commented Apr 16, 2024

Summary

slide: true を使用した際に drawio が図として表示されない問題の修正

Task

Issue

#8662

apps/app/src/components/Page/RevisionRenderer.tsx Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

RevisionRenderer は Comment や CustomSidebar からも使われているので、このコンポーネントに SlideViewer との切り替え機能を持たせるのは不適切

Copy link
Contributor Author

@reiji-h reiji-h Apr 23, 2024

Choose a reason for hiding this comment

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

こちらの対応として現在次の方法を考えています。

  1. RevisionRenderer 内で切り替える
    • RendererOptions に新たに isEnableSlide: boolean 等のスライドを使用できるか判別する key を導入して、今回同様 RevisionRenderer で判別、切り替えを行う。
  2. RevisionRenderer を使用するコンポーネントで切り替える。
    • 例えば PageEditor, PageView 等は RevisionRenderer を使用しているが、スライドを使用できるため、これらに切り替えの機能をもたせる。Comment 等はそのまま SlideViewer を用いる。

どちらの方が良い or それ以外の方法の方が良い等教えていただければ幸いです。

Copy link
Member

Choose a reason for hiding this comment

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

2がよいです

Copy link
Contributor Author

@reiji-h reiji-h Apr 30, 2024

Choose a reason for hiding this comment

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

2 で実装しました。
もともとスライドが使用可能だった rendererOptions は useViewOpitons と usePreviewOptions が返すものであり、前者は PageView.tsx と ShareLinkPageView.tsx が使用、後者は最終的に Preview.tsx のみにおいて使用されていました。よってこれら3つのファイルで切り替えをするようにしました。
ですが、PageView.tsx と ShareLinkPageView.tsx についてはこれらから直接 @growi/presentation を使用するとサーバーサイドからの import になるのかエラーが起こるので、dynamic import するコンポーネント ViewRenderer を噛ます形にしています。

@@ -40,6 +38,7 @@ const Comments = dynamic<CommentsProps>(() => import('../Comments').then(mod =>
const UsersHomepageFooter = dynamic<UsersHomepageFooterProps>(() => import('../UsersHomepageFooter')
.then(mod => mod.UsersHomepageFooter), { ssr: false });
const IdenticalPathPage = dynamic(() => import('../IdenticalPathPage').then(mod => mod.IdenticalPathPage), { ssr: false });
const ViewRenderer = dynamic(() => import('./ViewRenderer').then(mod => mod.ViewRenderer), { ssr: false });
Copy link
Member

Choose a reason for hiding this comment

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

ここで dynamic import してしまうと本文が SSR されなくなるので NG

Copy link
Contributor Author

Choose a reason for hiding this comment

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

確かに RevisionRenderer は SSR したかったので、SlideRenderer という dynamic import するコンポーネントを新たに作成し、それの children として RevisionRenderer を渡すようにしました。SlideRenderer 内ではスライドにするかどうかの判定を行いスライドにするなら SlideViewer を使用し、そうでなければ children (この場合 RevisionRenderer )を返すようにしています。

参考:https://qiita.com/kay-adamof/items/e3767e17e5be0b41faa8#2---server-component-%E3%82%92%E7%9B%B4%E6%8E%A5%E3%83%8D%E3%82%B9%E3%83%88%E3%81%97%E3%81%AA%E3%81%84

@@ -23,7 +22,7 @@ const logger = loggerFactory('growi:Page');

const PageSideContents = dynamic<PageSideContentsProps>(() => import('./PageSideContents').then(mod => mod.PageSideContents), { ssr: false });
const ForbiddenPage = dynamic(() => import('./ForbiddenPage'), { ssr: false });

const ViewRenderer = dynamic(() => import('./Page/ViewRenderer').then(mod => mod.ViewRenderer), { ssr: false });
Copy link
Member

Choose a reason for hiding this comment

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

ここで dynamic import してしまうと本文が SSR されなくなるので NG

Copy link
Contributor Author

Choose a reason for hiding this comment

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

確かに RevisionRenderer は SSR したかったので、SlideRenderer という dynamic import するコンポーネントを新たに作成し、それの children として RevisionRenderer を渡すようにしました。SlideRenderer 内ではスライドにするかどうかの判定を行いスライドにするなら SlideViewer を使用し、そうでなければ children (この場合 RevisionRenderer )を返すようにしています。

参考:https://qiita.com/kay-adamof/items/e3767e17e5be0b41faa8#2---server-component-%E3%82%92%E7%9B%B4%E6%8E%A5%E3%83%8D%E3%82%B9%E3%83%88%E3%81%97%E3%81%AA%E3%81%84


<SlideRenderer markdown={markdown}>
<RevisionRenderer rendererOptions={rendererOptions} markdown={markdown} />
</SlideRenderer>
Copy link
Member

Choose a reason for hiding this comment

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

これでは SSR されないのでだめです
ブラウザのネットワークタブで、レスポンス中に RevisionRenderer でレンダリングされるべき HTML が入っているかどうかを確認してください。

rendererOptions={rendererOptions}
markdown={markdown}
/>
</SlideRenderer>
Copy link
Member

Choose a reason for hiding this comment

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

こちらも PageView.tsx と同様 SSR されないので NG

This reverts commit d6eff20.
Copy link

reg-suit bot commented May 13, 2024

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 merged commit 0bbe98b into master May 13, 2024
21 of 24 checks passed
@yuki-takei yuki-takei deleted the fix/144087-144091-use-drawio-in-slide-view branch May 13, 2024 09:16
@github-actions github-actions bot mentioned this pull request May 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

None yet

2 participants