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

fix(*): remove vue-bind-once [KHCP-14269] #2535

Merged
merged 18 commits into from
Dec 12, 2024

Conversation

portikM
Copy link
Member

@portikM portikM commented Dec 5, 2024

Summary

Addresses: https://konghq.atlassian.net/browse/KHCP-14269

Remove dependancy on vue-bind-once. In all components, rely on Vue useId instead of nanoid for generating unique values for accessibility attributes.

@portikM portikM self-assigned this Dec 5, 2024
Copy link

netlify bot commented Dec 5, 2024

Deploy Preview for kongponents-sandbox ready!

Name Link
🔨 Latest commit 092d345
🔍 Latest deploy log https://app.netlify.com/sites/kongponents-sandbox/deploys/675b0f16542ea20008193734
😎 Deploy Preview https://deploy-preview-2535--kongponents-sandbox.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Dec 5, 2024

Deploy Preview for kongponents ready!

Name Link
🔨 Latest commit 092d345
🔍 Latest deploy log https://app.netlify.com/sites/kongponents/deploys/675b0f16f7e201000845de00
😎 Deploy Preview https://deploy-preview-2535--kongponents.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@portikM portikM changed the title fix(*): remove vue-bind-once and nanoid [KHCP-14269] fix(*): remove vue-bind-once [KHCP-14269] Dec 5, 2024
@portikM portikM marked this pull request as ready for review December 5, 2024 22:02
@portikM portikM requested a review from adamdehaven December 9, 2024 18:58
docs/guide/contributing.md Outdated Show resolved Hide resolved
docs/guide/contributing.md Outdated Show resolved Hide resolved
@portikM portikM requested a review from adamdehaven December 10, 2024 17:07
@portikM portikM enabled auto-merge (squash) December 10, 2024 23:03
@@ -110,7 +109,7 @@ const emit = defineEmits<{
const slots = useSlots()
const attrs = useAttrs()

const inputId = attrs.id ? String(attrs.id) : useUniqueId()
const inputId = attrs.id ? String(attrs.id) : useId()
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const inputId = attrs.id ? String(attrs.id) : useId()
const id = useId()
const inputId = computed(() => attrs.id ? String(attrs.id) : id)

I think now this should be a computed ref as we now support dynamic user defined id.

Copy link
Member

Choose a reason for hiding this comment

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

nit: I'd name differently than id just to not get confused with other properties, etc. Something like defaultId might be more obvious

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

@@ -134,6 +134,8 @@ const emit = defineEmits<{

const { stripRequiredLabel } = useUtilities()

const fileInputId = attrs.id ? String(attrs.id) : useId()
Copy link
Contributor

Choose a reason for hiding this comment

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

computed

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

@@ -175,8 +175,8 @@ const slots = useSlots()
const attrs = useAttrs()

const isRequired = computed((): boolean => attrs?.required !== undefined && String(attrs?.required) !== 'false')
const inputId = attrs.id ? String(attrs.id) : useUniqueId()
const helpTextId = useUniqueId()
const inputId = attrs.id ? String(attrs.id) : useId()
Copy link
Contributor

Choose a reason for hiding this comment

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

computed

Copy link
Member

Choose a reason for hiding this comment

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

You could make these computed, but the useId() call would need to be made outside of the computed function and then referenced, similar to this:

const defaultId = useId()
const inputId = computed((): string => attrs.id ? String(attrs.id) : defaultId)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I have an example in my first comment.

Copy link
Member

Choose a reason for hiding this comment

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

Ah yep I missed that one 👍🏼

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

@@ -100,7 +99,7 @@ const attrs = useAttrs()

const switchInputElement = ref<HTMLInputElement | null>(null)

const inputId = attrs.id ? String(attrs.id) : useUniqueId()
const inputId = attrs.id ? String(attrs.id) : useId()
Copy link
Contributor

Choose a reason for hiding this comment

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

computed

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

@@ -495,8 +495,8 @@ const defaultKPopAttributes = {
const key = ref(0)
const stagingKey = ref(0)

const multiselectWrapperId = attrs.id ? String(attrs.id) : useUniqueId() // unique id for the KLabel `for` attribute
const multiselectKey = useUniqueId()
const multiselectWrapperId = attrs.id ? String(attrs.id) : useId() // unique id for the KLabel `for` attribute
Copy link
Contributor

Choose a reason for hiding this comment

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

computed

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

@@ -177,7 +176,7 @@ const props = defineProps({
const slots = useSlots()
const attrs = useAttrs()

const inputId = attrs.id ? String(attrs.id) : useUniqueId()
const inputId = attrs.id ? String(attrs.id) : useId()
Copy link
Contributor

Choose a reason for hiding this comment

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

computed

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

@@ -363,6 +364,8 @@ const emit = defineEmits<{
const attrs = useAttrs()
const slots = useSlots()

const selectInputId = attrs.id ? String(attrs.id) : useId()
Copy link
Contributor

Choose a reason for hiding this comment

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

computed

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

@@ -161,7 +160,7 @@ const value = computed({
},
})

const textAreaId = attrs.id ? String(attrs.id) : useUniqueId()
const textAreaId = attrs.id ? String(attrs.id) : useId()
Copy link
Contributor

Choose a reason for hiding this comment

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

computed

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

@portikM portikM disabled auto-merge December 11, 2024 15:05
@portikM portikM marked this pull request as draft December 11, 2024 15:05
@portikM portikM requested a review from Justineo December 12, 2024 15:53
@portikM portikM marked this pull request as ready for review December 12, 2024 15:53
Copy link
Member

@adamdehaven adamdehaven left a comment

Choose a reason for hiding this comment

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

lgtm

@portikM portikM enabled auto-merge (squash) December 12, 2024 16:28
@portikM portikM merged commit 6d31fae into main Dec 12, 2024
11 checks passed
@portikM portikM deleted the fix/khcp-14269-remove-vue-bind-once-and-nanoid branch December 12, 2024 16:33
kongponents-bot pushed a commit that referenced this pull request Dec 12, 2024
## [9.14.22](v9.14.21...v9.14.22) (2024-12-12)

### Bug Fixes

* remove vue-bind-once [KHCP-14269] ([#2535](#2535)) ([6d31fae](6d31fae))
@kongponents-bot
Copy link
Collaborator

🎉 This PR is included in version 9.14.22 🎉

The release is available on:

Your semantic-release bot 📦🚀

fabianrbz added a commit to Kong/developer.konghq.com that referenced this pull request Dec 16, 2024
fabianrbz added a commit to Kong/developer.konghq.com that referenced this pull request Dec 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants