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: Improve Giscus theme visibility and add i18n support #127

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Seol-JY
Copy link
Contributor

@Seol-JY Seol-JY commented Jan 20, 2025

  • Fix UI clarity by updating Giscus theme to tritanopia
  • Add dynamic language support for comment section
  • Ensure dark/light mode is properly applied after giscus iframe is fully loaded

Theme Comparison

  • noborder theme (Before) vs tritanopia theme (After)
  • Please review both themes and let me know your preference:
    light
    dark

Language Support

  • Dynamic language switching based on user preferences
    lang

Theme consistency

  • Maintain theme consistency by applying giscus theme after load
    consistency

Copy link

vercel bot commented Jan 20, 2025

@Seol-JY is attempting to deploy a commit to the Toss Team on Vercel.

A member of the Team first needs to authorize it.

@Seol-JY Seol-JY changed the title eat: improve Giscus theme visibility and add i18n support Feat: Improve Giscus theme visibility and add i18n support Jan 20, 2025
@@ -27,10 +40,10 @@ watch(isDark, () => {
data-reactions-enabled="1"
data-emit-metadata="0"
data-input-position="bottom"
data-lang="en"
:data-lang="lang"
crossorigin="anonymous"
data-loading="lazy"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
data-loading="lazy"

If the user changes the theme before the giscus iframe is loaded, the postMessage does not take effect.

How about removing that line to disable lazy loading?

Copy link
Contributor Author

@Seol-JY Seol-JY Jan 21, 2025

Choose a reason for hiding this comment

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

I've identified the issue! It would be great if we could get the Maintainer's thoughts on this part.

While it seems possible to resolve this issue without removing the lazy loading option, it would likely require some tricky workarounds

Copy link
Member

@Kimbangg Kimbangg Jan 22, 2025

Choose a reason for hiding this comment

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

@Seol-JY @ohprettyhak
I've resolved it by applying the theme again when the load is complete. I used addEventListener("event", () => {}) to update the theme once the iframe has finished loading. 🙏

Since I'm not very familiar with Vue or Giscus, please let me know if there's a better way to approach this! 👀

2025-01-22.8.41.01.mov
// component
<script setup lang="ts">
import { useData } from "vitepress";
import { useGiscusTheme } from "../hooks/useGiscusTheme";

const { frontmatter, title, isDark } = useData();
useGiscusTheme();
</script>

<template>
  <div
    v-if="frontmatter.comments !== false"
    :key="title"
    class="giscus"
    style="margin-top: 24px"
  >
    <component
      :is="'script'"
      :data-theme="isDark ? 'noborder_dark' : 'noborder_light'"
      src="https://giscus.app/client.js"
      data-repo="toss/frontend-fundamentals"
      data-repo-id="R_kgDONfHk5g"
      data-category="Commented Docs"
      data-category-id="DIC_kwDONfHk5s4ClUqX"
      data-mapping="title"
      data-strict="0"
      data-reactions-enabled="1"
      data-emit-metadata="0"
      data-input-position="bottom"
      data-lang="en"
      crossorigin="anonymous"
      data-loading="lazy"
      async
    />
  </div>
</template>

// useGiscusTheme.tsx
import { ref, watch, computed } from "vue";
import { useData } from "vitepress";

import { sendGiscusMessage, GISCUS_THEME } from "../utils";

export function useGiscusTheme() {
  const { isDark } = useData();
  const isIframeLoaded = ref(false);

  const setupThemeHandler = () => {
    window.addEventListener("message", (event) => {
      if (event.origin === "https://giscus.app" && event.data?.giscus != null) {
        isIframeLoaded.value = true;
        syncTheme();
      }
    });
  };

  const syncTheme = () => {
    sendGiscusMessage({
      setConfig: {
        theme: isDark.value ? GISCUS_THEME.dark : GISCUS_THEME.light
      }
    });
  };

  watch(isDark, () => {
    if (isIframeLoaded.value) {
      syncTheme();
    }
  });

  setupThemeHandler();
  
 /**
 * optional
 /* 
  return {
    theme: computed(() =>
      isDark.value ? GISCUS_THEME.dark : GISCUS_THEME.light
    )
  };
}

const GISCUS_ORIGIN = "https://giscus.app" as const;

export const GISCUS_THEME = {
  light: "noborder_light",
  dark: "noborder_dark"
};

export function sendGiscusMessage<T>(message: T) {
  const iframe = document.querySelector<HTMLIFrameElement>(
    "iframe.giscus-frame"
  );

  if (iframe == null) {
    return;
  }

  iframe.contentWindow?.postMessage(
    {
      giscus: message
    },
    GISCUS_ORIGIN
  );
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good to me 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Kimbangg
I've implemented the functionality based on your suggested code! I've updated the PR content accordingly. While this is unrelated to the current PR, I'm wondering about your thoughts on introducing Toss Product Sans font for the document content (body text only) in the next PR?

@Seol-JY Seol-JY requested a review from Kimbangg January 22, 2025 14:57
Comment on lines +3 to +7
export const GISCUS_LANG_MAP = {
ko: "ko",
en: "en",
ja: "ja"
} as const;
Copy link
Contributor Author

@Seol-JY Seol-JY Jan 22, 2025

Choose a reason for hiding this comment

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

Since site language codes and giscus language codes may be different from each other and this can cause immediate errors, this work was done to handle that mapping.

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.

3 participants