feat: enable turning off code ligatures#2476
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
|
Hello! Thank you for opening your first PR to npmx, @Kiwow! 🚀 Here’s what will happen next:
|
Lunaria Status Overview🌕 This pull request will trigger status changes. Learn moreBy default, every PR changing files present in the Lunaria configuration's You can change this by adding one of the keywords present in the Tracked Files
Warnings reference
|
Kiwow
changed the title
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Kiwow
force-pushed
the
enable-disabling-code-ligatures
branch
from
84b65b6 to
bd5155c
Compare
📝 WalkthroughWalkthroughAdds a settings toggle to enable/disable code ligatures. Introduces Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/composables/useSettings.ts (1)
260-278: Wrap withcreateSharedComposableto align with codebase patterns and prevent future duplicate watchers.This composable follows the same pattern as
useKeyboardShortcuts(computed property + immediate watcher on a settings field). Currently only one production consumer exists insettings.vue, but wrapping withcreateSharedComposableensures consistent behaviour if additional call sites are added, preventing duplicate side effects.♻️ Suggested refactor
-export function useCodeLigatures() { +export const useCodeLigatures = createSharedComposable(function useCodeLigatures() { const { settings } = useSettings() const codeLigatures = computed(() => settings.value.codeLigatures) if (import.meta.client) { // Sync the data attribute on root to the setting watch( codeLigatures, value => { if (value) { delete document.documentElement.dataset.codeLigatures } else { document.documentElement.dataset.codeLigatures = 'false' } }, { immediate: true }, ) } function toggleCodeLigatures() { settings.value.codeLigatures = !settings.value.codeLigatures } return { codeLigatures, toggleCodeLigatures, } -} +})
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2b8ad8a1-e6d7-4a95-9bd4-f6f4bae29cec
📒 Files selected for processing (7)
app/assets/main.cssapp/composables/useSettings.tsapp/pages/settings.vueapp/utils/prehydrate.tsi18n/locales/cs-CZ.jsoni18n/locales/en.jsontest/nuxt/composables/use-settings.spec.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/composables/useSettings.ts (1)
260-288: WrapuseCodeLigatureswithcreateSharedComposablefor consistency with similar side effects.This composable performs a global DOM mutation via
watch, identical touseKeyboardShortcutswhich already usescreateSharedComposable. Whilst currently called only from settings.vue, wrapping it ensures singleton behaviour and aligns with the established pattern in the codebase.♻️ Proposed refactor
-export function useCodeLigatures() { +export const useCodeLigatures = createSharedComposable(function useCodeLigatures() { const { settings } = useSettings() const codeLigatures = computed(() => settings.value.codeLigatures) if (import.meta.client) { // Sync the data attribute on root to the setting watch( codeLigatures, value => { if (value) { delete document.documentElement.dataset.codeLigatures } else { document.documentElement.dataset.codeLigatures = 'false' } }, { immediate: true }, ) } function toggleCodeLigatures() { settings.value.codeLigatures = !settings.value.codeLigatures } return { codeLigatures, toggleCodeLigatures, } -} +})
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 18833c17-f4e9-46b8-abec-daf82e5eb208
📒 Files selected for processing (8)
app/assets/main.cssapp/composables/useSettings.tsapp/pages/settings.vueapp/utils/prehydrate.tsi18n/locales/cs-CZ.jsoni18n/locales/en.jsoni18n/schema.jsontest/nuxt/composables/use-settings.spec.ts
✅ Files skipped from review due to trivial changes (3)
- app/assets/main.css
- test/nuxt/composables/use-settings.spec.ts
- i18n/locales/cs-CZ.json
🚧 Files skipped from review as they are similar to previous changes (3)
- app/pages/settings.vue
- app/utils/prehydrate.ts
- i18n/locales/en.json
|
I don't totally understand
EDIT: After thinking about it a bit more, I think it makes sense to use it here, so I've done so |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/composables/useSettings.ts (1)
265-277: Consider a tiny helper to deduplicate root dataset boolean syncing.This watch block duplicates the same pattern used in
useKeyboardShortcuts(set attribute to'false'when disabled, remove when enabled). Extracting that pattern into a small helper would reduce repetition and keep future flag additions simpler.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d7bdbaf0-8237-422e-9506-a313f501c8c8
📒 Files selected for processing (1)
app/composables/useSettings.ts
| } | ||
| } | ||
|
|
||
| export const useCodeLigatures = createSharedComposable(function useCodeLigatures() { |
There was a problem hiding this comment.
| export const useCodeLigatures = createSharedComposable(function useCodeLigatures() { | |
| export const useCodeLigatures = createSharedComposable(() => { |
There was a problem hiding this comment.
I saw this in the useKeyboardShortcuts composable and it made sense to me as a way to get nicer stack traces in logs. I think it's nice to have a named function for that reason.
To be fair, in prod builds, it won't make a difference, so I'm up for either solution. Your version is definitely more readable.
To compare, here's the outputs of `console.trace` (running in dev) when called at the start of the composable and the start of the watcher function, respectively:
Named function expression:
Anonymous arrow function:
3 participants
Closes #2423
Adds a new setting that controls whether ligatures are used in code. The setting sets a data attribute on the root, which is picked up by a selector in global styles.
I opted to have changes in the setting call the
toggleCodeLigaturesfunction rather than usingv-model. This is needed because we need to run the side effect of updating the data attribute, which happens inside awatchis registered inuseCodeLigatures. I still don't really like this, because the attribute update is dependent onuseCodeLigaturesbeing called, even though you can access and update the setting itself onsettings. Other settings work like this as well though (e.g. the data attribute forkeyboardShortcutsgets updated only ifuseKeyboardShortcutsis called), so for the time being, I've kept it this way. I'd be interested in revisiting this in the future, perhaps updating theuseSettingscomposable to handle side effects on changes of individual settings. Please let me know if that makes sense to you.I've provided translations for the new setting's label in English and Czech (my native language).