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

chore: replace redundant useManagedState with Vue's defineModel #2529

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

Conversation

JoCa96
Copy link
Collaborator

@JoCa96 JoCa96 commented Jan 16, 2025

The changes are mostly internal, but the typings were of OnyxSelect have changed:

  • The modelValue now infers a specific subtype of SelectOptionValue and the options values must match.
  • withSearch: Filtering of the options will not automatically disabled anymore when searchTerm is bound. Instead noFilter must be set.
  • removed Arrayable type, imported from vitest, as it was an internal type. Replaced with our own type with the same name.
  • removed redundant MaybeReactiveSource type, as it is now covered by Vue's MaybeRefOrGetter type

Copy link

changeset-bot bot commented Jan 16, 2025

🦋 Changeset detected

Latest commit: 21148a0

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 7 packages
Name Type
@sit-onyx/headless Patch
sit-onyx Major
demo-app Patch
playground Patch
@sit-onyx/chartjs-plugin Major
@sit-onyx/nuxt Major
@sit-onyx/vitepress-theme Major

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@JoCa96 JoCa96 force-pushed the joca96/remove-managed-state branch 3 times, most recently from 42ee9e0 to 7deab1a Compare January 16, 2025 10:36
@JoCa96 JoCa96 changed the title replaced managed state with defineModel chore: replace redundant useManagedState with Vue's defineModel Jan 16, 2025
@JoCa96 JoCa96 marked this pull request as ready for review January 16, 2025 12:32
@JoCa96 JoCa96 requested a review from a team as a code owner January 16, 2025 12:32
packages/sit-onyx/src/components/OnyxSelect/OnyxSelect.vue Outdated Show resolved Hide resolved
@@ -3,6 +3,7 @@
"include": ["src/**/*"],
"compilerOptions": {
"rootDir": "./src",
"lib": ["DOM", "DOM.Iterable", "ES2022"],
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this change needed? Same question for the playground

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Honestly, I don't know why it is needed now.
Without it, I am getting errors:
Screenshot 2025-01-17 at 08 58 30

The errors make sense, but why only after all these seemingly unrelated changes?
My assumption is, that the Vue compiler can handle the updated typings better and doesn't bail out (as early)?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm shouldn't the headless code be inlined? Maybe this has something to do with the update to Vite 6 in #2530 ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

JavaScript is inlined, the typings are not inlined afaik

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think it is related, because there are no issues on main

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should find out why and fix this, otherwise propably all projects using onyx will have build errors...

@larsrickert larsrickert self-assigned this Jan 17, 2025
@JoCa96 JoCa96 force-pushed the joca96/remove-managed-state branch from a77be86 to 77d2951 Compare January 17, 2025 08:03
@JoCa96 JoCa96 requested a review from larsrickert January 17, 2025 08:18
@@ -424,7 +423,6 @@ defineExpose({ input: computed(() => selectInput.value?.input) });
value: option.value,
label: option.label,
disabled: option.disabled,
// TODO: remove type cast once its fixed in Vue / vue-tsc version
selected: arrayValue.some((value: TValue) => value === option.value),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
selected: arrayValue.some((value: TValue) => value === option.value),
selected: arrayValue.some((value) => value === option.value),

I guess the TODO was meant for this one

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the type hint here is still necessary

@JoCa96 JoCa96 force-pushed the joca96/remove-managed-state branch from 6138353 to 21148a0 Compare January 17, 2025 12:04
@JoCa96 JoCa96 requested a review from larsrickert January 17, 2025 12:05
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.

2 participants