-
-
Notifications
You must be signed in to change notification settings - Fork 6
Vanilla state management for language chooser #113
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
base: main
Are you sure you want to change the base?
Conversation
imnasnainaec
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 19 files reviewed, all discussions resolved
components/language-chooser/common/language-chooser-controller/src/state-management/field.ts line 14 at r1 (raw file):
* Callback to update the UI when the field changes */ onUpdate: ((newValue: T) => void) | null = null;
What is onUpdate for? It's never set within this class (or even within this pr). Should it be explicitly public? Or made private and added to the constructor analogously to onUpdateRequested?
components/language-chooser/common/language-chooser-controller/src/selectable.ts line 10 at r1 (raw file):
for (let i = 0; i < items.length; i++) { items[i].isSelected.value = i === index; }
What about something like
items.forEach((item, i) => {
item.isSelected.value = i === index;
});
|
@imnasnainaec Thanks for the feedback. |
imnasnainaec
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@imnasnainaec reviewed 3 of 34 files at r2, all commit messages.
Reviewable status: 3 of 54 files reviewed, all discussions resolved
components/language-chooser/common/language-chooser-controller/src/view-models/language-card.ts line 17 at r3 (raw file):
if (onSelect) { onSelect(isSelected); }
This can be shortened to onSelect?.(isSelected);, though that's a stylistic choice.
components/language-chooser/common/language-chooser-controller/src/view-models/script-card.ts line 17 at r3 (raw file):
if (onSelect) { onSelect(isSelected); }
This can be shortened to onSelect?.(isSelected);, though that's a stylistic choice.
components/state-management/state-management-core/src/field.ts line 29 at r3 (raw file):
if (this._onUpdateRequested) { this._onUpdateRequested(value, oldValue); }
This can be shortened to this._onUpdateRequested?.(value, oldValue);, though that's a stylistic choice.
components/state-management/state-management-core/src/field.ts line 41 at r3 (raw file):
public set value(value: T) { try { if (this.onUpdate) this.onUpdate(value);
This can be shortened to this.onUpdate?.(value);, though that's a stylistic choice.
eb8855b to
3bee4a1
Compare
|
The Svelte demo app is now functional and almost bug-free when run on the Vite development server. However, minification will likely break the app in production. Still making some changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR introduces a framework-agnostic state management system for a language chooser component, including a core library, Svelte adapter, controller logic, and a demo application using DaisyUI.
Key Changes:
- New state management core library with a
Fieldclass for reactive data binding - Svelte adapter that transforms vanilla view models into Svelte-compatible reactive properties
- Language chooser controller with view models for language selection, script selection, and customization
- Complete Svelte+DaisyUI implementation with demo application
Reviewed changes
Copilot reviewed 63 out of 64 changed files in this pull request and generated 22 comments.
Show a summary per file
| File | Description |
|---|---|
| package.json | Added new workspace entries for state-management and svelte packages |
| nx.json | Updated project list to include new packages in alphabetical order |
| eslint.config.mjs | Added ignore patterns for vite/vitest timestamp files and fixed trailing comma |
| components/state-management/state-management-core/* | Core state management library with Field class, tests, and documentation |
| components/state-management/state-management-svelte/* | Svelte adapter for transforming view models with reactive fields |
| components/language-chooser/common/language-chooser-controller/* | Framework-agnostic view models and logic for language selection |
| components/language-chooser/svelte/language-chooser-svelte-daisyui/* | Svelte components and demo app using DaisyUI for styling |
| .vscode/extensions.json | Added Prettier extension recommendation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| }, | ||
| lib: { | ||
| entry: "./index.ts", | ||
| name: "@ethnolib/find-language", |
Copilot
AI
Dec 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The library name in the vite config should be "@ethnolib/state-management-svelte" but it's currently set to "@ethnolib/find-language". This will cause incorrect naming in the built output.
| function _appendLanguages(languages: ILanguage[]) { | ||
| const baseIndex = listedLanguages.value.length; | ||
| const newLanguages = languages.map((lang, i) => | ||
| useLanguageChardViewModel(lang, { |
Copilot
AI
Dec 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo in function name: "useLanguageChardViewModel" should be "useLanguageCardViewModel" (missing 'a' in "Card"). This should match the corrected spelling in the function definition.
| }); | ||
|
|
||
| describe("selected script", () => { | ||
| it("should match scipt selection", () => { |
Copilot
AI
Dec 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo in "scipt" should be "script" (note: there's a typo in the test description).
| it("should match scipt selection", () => { | |
| it("should match script selection", () => { |
| @@ -0,0 +1,108 @@ | |||
| # State Management Core | |||
|
|
|||
| This package provides a simple interface for working with multiple reactive framworks. | |||
Copilot
AI
Dec 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo in "framworks" should be "frameworks" (note: there's a typo in the README documentation).
| }, | ||
| lib: { | ||
| entry: "./index.ts", | ||
| name: "@ethnolib/find-language", |
Copilot
AI
Dec 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The library name in the vite config should be "@ethnolib/state-management-core" but it's currently set to "@ethnolib/find-language". This will cause incorrect naming in the built output.
|
|
||
| class TestHelper { | ||
| constructor(language: ILanguage) { | ||
| this.card = useLanguageChardViewModel(language); |
Copilot
AI
Dec 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo in function name: "useLanguageChardViewModel" should be "useLanguageCardViewModel" (missing 'a' in "Card"). This should match the corrected spelling in the function definition.
| }); | ||
| } | ||
|
|
||
| function submitCustomizeLangaugeModal({ |
Copilot
AI
Dec 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo in function name: "submitCustomizeLangaugeModal" should be "submitCustomizeLanguageModal" (missing 'a' in "Language").
| function submitCustomizeLangaugeModal({ | |
| function submitCustomizeLanguageModal({ |
| "devDependencies": { | ||
| "@nx/vite": "^19.1.2", | ||
| "@types/node": "^20.16.11", | ||
| "svelte": "^5.38.6", | ||
| "svelte-check": "^4.0.0", | ||
| "@sveltejs/vite-plugin-svelte": "^4.0.0", | ||
| "tsx": "^4.19.2", | ||
| "typescript": "^5.2.2" | ||
| }, | ||
| "volta": { | ||
| "extends": "../../../package.json" | ||
| } |
Copilot
AI
Dec 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing dependency: The package depends on "@ethnolib/state-management-core" but this is not listed in the dependencies or devDependencies. Add it to ensure the package can be properly installed and used.
| "devDependencies": { | ||
| "@nx/vite": "^19.1.2", | ||
| "@types/node": "^20.16.11", | ||
| "tsx": "^4.19.2", | ||
| "typescript": "^5.2.2" | ||
| }, | ||
| "volta": { | ||
| "extends": "../../../../package.json" | ||
| } |
Copilot
AI
Dec 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing dependencies: The package depends on "@ethnolib/find-language" and "@ethnolib/state-management-core" but these are not listed in the dependencies or devDependencies. Add them to ensure the package can be properly installed and used.
| "devDependencies": { | ||
| "@nx/vite": "^20.4.6", | ||
| "@sveltejs/vite-plugin-svelte": "^6.1.4", | ||
| "@tailwindcss/postcss": "^4.1.13", | ||
| "@tailwindcss/typography": "^0.5.16", | ||
| "@tsconfig/svelte": "^5.0.0", | ||
| "autoprefixer": "^10.4.21", | ||
| "daisyui": "^5.1.12", | ||
| "eslint-plugin-svelte": "^2.44.0", | ||
| "postcss": "^8.5.6", | ||
| "svelte": "^5.38.6", | ||
| "svelte-check": "^4.0.5", | ||
| "svelte-preprocess": "^6.0.0", | ||
| "tailwindcss": "^3.4.17" | ||
| }, | ||
| "engines": { | ||
| "node": ">=18.18" | ||
| }, | ||
| "nx": {} |
Copilot
AI
Dec 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing dependencies: The package depends on "@ethnolib/find-language", "@ethnolib/language-chooser-controller", and "@ethnolib/state-management-svelte" but these are not listed in the dependencies or devDependencies. Add them to ensure the package can be properly installed and used.
A framework-agnostic view model for a language chooser.
The goal of this project is to make it easy to create a language chooser in any reactive framework (React, Svelte, Vue, etc).
TODO:
This change is