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

refactor(checkbox): update component #432

Merged
merged 26 commits into from
Nov 29, 2023

Conversation

Cr0zy07
Copy link
Collaborator

@Cr0zy07 Cr0zy07 commented Nov 27, 2023

Description

Linked Issues

#358

Additional context

Copy link
Contributor

github-actions bot commented Nov 27, 2023

Thank you for following the naming conventions! 🙏

@Cr0zy07 Cr0zy07 changed the title refactor(avatar): update component refactor(checkbox): update component Nov 27, 2023
@Cr0zy07 Cr0zy07 marked this pull request as ready for review November 28, 2023 13:05
@@ -32,16 +33,11 @@ const popoverAnchor = defineComponent({

return () => h(OkuPopperAnchor, {
...popperScope,
...mergeProps(attrs, reactiveAnchorProps),
...mergeProps(attrs, otherProps),
Copy link
Member

Choose a reason for hiding this comment

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

Don't we need to add emits, you stopped coming up here? useListener()

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will do it in a separate PR, I did not focus or edit much in the other components except the render function.

@productdevbook
Copy link
Member

Thank you for this great work ❤️

@productdevbook productdevbook merged commit 545bb06 into oku-ui:main Nov 29, 2023
3 checks passed
@github-actions github-actions bot mentioned this pull request Nov 29, 2023
@@ -40,7 +40,7 @@ const Checkbox = defineComponent({
const composedRefs = useComposedRefs(forwardedRef, buttonRef)
const hasConsumerStoppedPropagationRef = ref(false)
// We set this to true by default so that events bubble to forms without JS (SSR)
const isFormControl = computed(() => buttonRef.value ? Boolean(buttonRef.value?.closest('form')) : true)
const isFormControl = computed(() => buttonRef.value instanceof HTMLElement ? Boolean(buttonRef.value.closest('form')) : false)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@productdevbook I accidentally changed this to false when I was fixing the vitest issue. Do you know why this happened when it is set to true?

vitest caught 1 unhandled error during the test run.
This might cause false positive tests. Resolve unhandled errors to make sure your tests are not affected.

buttonRef.value?.closest is not a function
❯ ReactiveEffect.fn packages/components/checkbox/src/checkbox.ts:43:85
41| const hasConsumerStoppedPropagationRef = ref(false)
42| // We set this to true by default so that events bubble to forms without JS (SSR)
43| const isFormControl = computed(() => buttonRef.value ? Boolean(buttonRef.value?.closest('form')) : tr…

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.

None yet

2 participants