Skip to content

Conversation

@gmjgeek
Copy link

@gmjgeek gmjgeek commented Aug 31, 2025

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:

  • Support custom language options
  • Expose "ready to submit" field
  • Implement Svelte adaptor for Field class
  • Build a demo Svelte app
  • Highlight search results
  • Support translation through Crowdin
  • Add warning/info icons
Screenshot 2025-09-20 at 8 30 48 PM

This change is Reviewable

Copy link

@imnasnainaec imnasnainaec left a 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;
});

@gmjgeek
Copy link
Author

gmjgeek commented Sep 2, 2025

@imnasnainaec Thanks for the feedback. Field.onUpdate is now explicitly public. This callback is a mechanism for updating the UI, which I'm now starting to implement.

Copy link

@imnasnainaec imnasnainaec left a 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.

@gmjgeek
Copy link
Author

gmjgeek commented Sep 21, 2025

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.

@gmjgeek gmjgeek marked this pull request as ready for review September 29, 2025 17:11
Copy link

Copilot AI left a 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 Field class 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.

@andrew-polk
Copy link
Contributor

components/state-management/state-management-core/src/field.ts line 45 at r6 (raw file):

    } finally {
      this._value = value;
    }

devinreview says

Field.value setter calls updateUI before setting _value

In field.ts:40-45, the updateUI callback is invoked before updating _value. This means during the callback, field.value still returns the old value. This is intentional based on the test at field.spec.ts:36-42 which expects this ordering. However, this could be surprising to consumers who might expect the value to be updated when updateUI is called. The SvelteFieldImpl at field.svelte.ts:9 handles this correctly by updating its own state independent of the Field's internal state.

Copy link
Contributor

@andrew-polk andrew-polk left a comment

Choose a reason for hiding this comment

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

@andrew-polk partially reviewed 77 files and all commit messages.
Reviewable status: all files reviewed, 23 unresolved discussions (waiting on @gmjgeek).

Copy link
Contributor

@andrew-polk andrew-polk left a comment

Choose a reason for hiding this comment

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

@andrew-polk partially reviewed 24 files and all commit messages, made 2 comments, resolved 13 discussions, and dismissed @copilot[bot] from 12 discussions.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @gmjgeek).


components/language-chooser/common/language-chooser-controller/src/view-models/language-chooser.ts line 154 at r7 (raw file):

    customizations.value.customDisplayName = displayName.value;
    _updateIsReadyToSubmit();
  }

devinreview says the below. Do we need a comment at least? Or is this pointing out a problem?


components/language-chooser/common/language-chooser-controller/src/view-models/language-chooser.ts:R150-154

_onDisplayNameChanged mutates customizations object without triggering _onCustomizationsChanged

At line 151, customizations.value ??= {} uses the nullish coalescing assignment with the .value setter, which triggers updateUI but NOT _onUpdateRequested (i.e., _onCustomizationsChanged). Then line 152 mutates the object in-place: customizations.value.customDisplayName = displayName.value. This means changing the display name does NOT trigger _onCustomizationsChanged which would set selectedLanguage.value ??= UNLISTED_LANGUAGE. This appears intentional — the display name change should not force an unlisted language selection. However, this pattern of mutating the object in-place means the updateUI callback won't fire for the mutation on line 152, so the Svelte binding for customizations won't be notified of the customDisplayName change. In practice this may not matter since displayName is separately tracked, but it's a subtle reactivity gap worth being aware of.


components/language-chooser/common/language-chooser-controller/src/view-models/language-chooser.ts line 267 at r7 (raw file):

      customDisplayName: customizations.value?.customDisplayName,
    });
    selectedScript.requestUpdate(script);

Sorry; devinreview picked up something else:


submitCustomizeLanguageModal updates script after tag preview and display name are recalculated, leaving them stale

When submitCustomizeLanguageModal is called, the tag preview and display name are computed with the old script value because selectedScript is updated after customizations.requestUpdate(...) triggers the recalculation.

In submitCustomizeLanguageModal (lines 262-268), the order of operations is:

  1. customizations.requestUpdate({...}) — this triggers _onCustomizationsChanged → _onOrthographyChanged → _updateTagPreview() and _updateDisplayName(), both of which read selectedScript.value (still the old value at this point).
  2. selectedScript.requestUpdate(script) — this updates selectedScript.value to the new script, but since selectedScript was created without an onUpdateRequested callback (new Field<IScript | undefined>(undefined) at line 41), no recalculation of tag preview or display name is triggered.

As a result, tagPreview and displayName reflect the old script, not the newly submitted one. The fix is to set selectedScript.value = script before calling customizations.requestUpdate(...), so that _onOrthographyChanged computes with the correct script.

Impact: After submitting the customize language modal with a different script, the language tag preview and display name shown to the user will be incorrect until some other action triggers a recalculation.

Suggested fix

    selectedScript.value = script;
    customizations.requestUpdate({
      region,
      dialect,
      customDisplayName: customizations.value?.customDisplayName,
    });
  }

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