Skip to content

Allowed setting id on location-select component#17080

Merged
snipe merged 2 commits intogrokability:developfrom
marcusmoore:allow-id-on-location-select
Aug 7, 2025
Merged

Allowed setting id on location-select component#17080
snipe merged 2 commits intogrokability:developfrom
marcusmoore:allow-id-on-location-select

Conversation

@marcusmoore
Copy link
Copy Markdown
Collaborator

@marcusmoore marcusmoore commented Jun 2, 2025

This PR allows customizing the id for the location-select component.

So far it is only used on the asset check in page and, since the name is set to location_id, the id is rendered as location_id_location_select. This behavior is still present but now passing something like id="here" would render id="here" within the component.

@marcusmoore marcusmoore requested a review from snipe as a code owner June 2, 2025 22:58
@snipe snipe requested a review from uberbrady June 11, 2025 09:29
@snipe
Copy link
Copy Markdown
Member

snipe commented Jun 25, 2025

Are we sure that won't conflict with any jQuery magic we're doing elsewhere?

@marcusmoore
Copy link
Copy Markdown
Collaborator Author

I can't think of anything conflicting. This is only allowing the option of passing in a custom id. The existing $name_location_select is still what is rendered on the asset check in page currently.

@marcusmoore
Copy link
Copy Markdown
Collaborator Author

Gentle ping on this one. This is component is only being used in one place so far and these changes only exist to provide future flexibility.

@snipe
Copy link
Copy Markdown
Member

snipe commented Aug 7, 2025

I think I worked around it anyway, so it's not really needed, but it won't hurt, I suppose.

@snipe snipe merged commit 67c931f into grokability:develop Aug 7, 2025
8 checks passed
@marcusmoore marcusmoore deleted the allow-id-on-location-select branch August 7, 2025 17:27
Yvejen pushed a commit to Yvejen/snipe-it that referenced this pull request Nov 19, 2025
…ation-select

Allowed setting `id` on location-select component
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