π¨ Palette: [UX improvement] μΈλΆ λ§ν¬μ μ μ°½ μ΄λ¦Ό ν΄ν(title) μΆκ° - #193
Conversation
|
π Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a π emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
No actionable comments were generated in the recent review. π βΉοΈ Recent review infoβοΈ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: π Files selected for processing (3)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. π WalkthroughWalkthroughμΈλΆ λ§ν¬μ μ μ°½ μ΄λ¦Ό μλ΄λ₯Ό μΆκ°νμ΅λλ€. νκ΅μ΄μ μμ΄ λ©μμ§λ₯Ό μ μνμ΅λλ€. ChangesμΈλΆ λ§ν¬ μ κ·Όμ± μλ΄
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: βͺ Minimal Β· up to The PR adds multilingual new-window guidance to external links through localized tooltip text. No actionable merge-blocking risk remains beyond normal checks and review. π₯ Pre-merge checks | β 4 | β 1β Failed checks (1 warning)
β Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files. (2 skipped: 2 unsupported.) β¨ Finishing Touches π‘ 1π Generate docstrings π‘
π§ͺ Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Devin Review found 3 potential issues.
β οΈ 1 issue in files not directly in the diff
β οΈ Missing CHANGELOG entry for user-facing change
CLAUDE.md requires CHANGELOG.md to be updated under [Unreleased] in Korean for user-facing changes. This PR adds external-link tooltips, a user-facing change, but adds no CHANGELOG entry.
|
|
||
| if (!isInitialDefault) { | ||
| if (!i18nNodes) { | ||
| i18nNodes = document.querySelectorAll("[data-i18n]"); | ||
| i18nNodes = document.querySelectorAll("[data-i18n], [data-i18n-title]"); | ||
| } | ||
|
|
||
| // Only update textContent if it actually changed to avoid layout recalculations | ||
| i18nNodes.forEach((node) => { | ||
| const newText = dict[node.dataset.i18n]; | ||
| if (newText && node.textContent !== newText) { | ||
| node.textContent = newText; | ||
| if (node.hasAttribute("data-i18n")) { | ||
| const newText = dict[node.dataset.i18n]; | ||
| if (newText && node.textContent !== newText) { | ||
| node.textContent = newText; | ||
| } | ||
| } | ||
| if (node.hasAttribute("data-i18n-title")) { | ||
| const newTitle = dict[node.dataset.i18nTitle]; | ||
| if (newTitle && node.getAttribute("title") !== newTitle) { | ||
| node.setAttribute("title", newTitle); | ||
| } | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
π Info: Initial Korean load skips title translation, stays consistent
On initial Korean load isInitialDefault skips the node traversal, so data-i18n-title titles are never set by JS. Safe because every external link carries an inline Korean title matching a11y.newWindow, and later language switches populate i18nNodes and update titles correctly.
(Refers to this code)
Was this helpful? React with π or π to provide feedback.
| **Learning:** Using an anchor tag to wrap an entire card (block-level element) can result in verbose and confusing screen reader output. However, restricting the clickable area to just the title makes the UI harder to interact with (violating Fitts's Law). | ||
| **Action:** Apply `position: relative` to the card container and use a `::after` pseudo-element with `position: absolute; inset: 0;` on the title's anchor tag. This expands the clickable area to the whole card while keeping semantic and accessible HTML structure. | ||
|
|
||
| ## 2024-08-25 - μΈλΆ λ§ν¬μ μ κ·Όμ± ν₯μμ μν title μμ±κ³Ό i18n μ μ© |
There was a problem hiding this comment.
π Info: Palette entry date likely off by two years
The new entry is dated 2024-08-25 while the change lands in 2026. Given the journal records recent incidents, this reads as a typo, though other entries also use 2024 dates so intent is ambiguous.
Was this helpful? React with π or π to provide feedback.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
π‘ Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 22927712ec
βΉοΈ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with π.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| <a href="#forks" data-i18n="nav.forks">Fork</a> | ||
| <a href="#work" data-i18n="nav.work">μμ </a> | ||
| <a target="_blank" rel="noopener noreferrer" href="https://github.com/ContextualWisdomLab">GitHub</a> | ||
| <a target="_blank" rel="noopener noreferrer" href="https://github.com/ContextualWisdomLab" title="(μ μ°½μμ μ΄λ¦Ό)" data-i18n-title="a11y.newWindow">GitHub</a> |
There was a problem hiding this comment.
For keyboard-only and touch users, the only new cue is a title tooltip, which is generally exposed through mouse hover and is not reliably announced by assistive technologies when the link already has visible text. Consequently, these users can still activate the link without learning that it opens a new tab, defeating the accessibility goal of this change; provide the localized hint through visible or visually hidden text, or associate it with the link using aria-describedby instead.
Useful? React with πΒ / π.
1 participant
π‘ What: μΈλΆ λ§ν¬(
target="_blank")μ λ€κ΅μ΄λ₯Ό μ§μνλtitleν΄νμ μΆκ°νμ¬ μ μ°½μμ μ΄λ¦Όμ μλ΄νλλ‘ κ°μ νμ΅λλ€.π― Why: μ€ν¬λ¦° 리λ λ° μκ° μ¬μ©μμκ² μΈλΆ λ§ν¬ μ΄λ μ μ νμ΄ μ΄λ¦°λ€λ λ§₯λ½μ μ 곡νμ¬ μΈμ§μ νΌλμ λ§κΈ° μν¨μ λλ€.
πΈ Before/After: ν΄λΉ μμ (ν΄νλ§ λ ΈμΆλ¨).
βΏ Accessibility: μ€ν¬λ¦° 리λκ° λ§ν¬λ₯Ό μ½μ λ μ μ°½ μ΄λ¦Ό μλ΄λ₯Ό λ³ν©νμ¬ κ³΅μ§ν μ μλλ‘ λ€κ΅μ΄(νκ΅μ΄/μμ΄) μ§μμ i18n ꡬ쑰μ νμ₯ μ μ©νμ΅λλ€.
PR created automatically by Jules for task 11946162662865325152 started by @seonghobae
Summary by CodeRabbit
μ κ·Όμ± κ°μ
λ¬Έμ