-
-
Notifications
You must be signed in to change notification settings - Fork 381
fix: ComboBox selected item visibility bug #4250
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
base: main
Are you sure you want to change the base?
fix: ComboBox selected item visibility bug #4250
Conversation
🦋 Changeset detectedLatest commit: 05cd879 The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AdeboyeDN typically when we see contributions that are using outdated information, it's due to AI contribution. We ask that users volunteer their use of LLMs as part of the PR submission process here:
#4250 (comment)
So either your LLM is using outdated information, or you have somehow draw information from an old documentation source. Make sure you're always using the current/modern docs at this URL:
https://www.skeleton.dev/
(Older docs have v2, v3, specified in the URL)
If you are making use of LLMs, that's fine, just make sure you:
- Disclose this information per our request (you can edit the PR post)
- Are feeding it the correct LLM source, which is available here:
https://www.skeleton.dev/docs/svelte/resources/llms
I'll go ahead and give you a chance to rectify this, but if the issue is not solved, and correct properties and protocols used, then I will have to deny and close this PR.
Thanks for your understanding.
|
Hi @endigo9740 , apologies, I’ve addressed your comments. Let me know what you think. |
endigo9740
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @AdeboyeDN - sorry for the delay. I'd submitted this review last week, but forgot to submit it. Let me know if you have any questions.
|
|
||
| &[data-highlighted] { | ||
| @apply preset-tonal; | ||
| color: var(--color-surface-contrast-950); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @AdeboyeDN thanks for the quick turn around here.
While this is a step in the right direction. I had mentioned the issue with using a fixed color and trying to address this in the combobox styles prior. For one, your solution improves dark mode - but here's light mode now:
Your current fix is aimed at addressing the issue localized to the combobox. If we were going to do that, we would need to use a color pairing to address both modes like so:
color: var(--color-surface-contrast-950-50);Which would use this value:
https://github.com/skeletonlabs/skeleton/blob/main/packages/skeleton/src/base/theme.css#L388
Unfortunately this is not a broad enough change. The core issue is that preset-tonal should in fact handle the color values automatically. This means the issue is in the preset itself. If we take a look here, we can see what I mean:
https://github.com/skeletonlabs/skeleton/blob/main/packages/skeleton/src/utilities/presets.css#L331
Unlike the other Tonal presets, the generic preset-tonal doesn't have color values set. So essentially we need to fix the color in the preset, not in the combobox styling:
@utility preset-tonal {
background-color: color-mix(in oklab, light-dark(var(--color-surface-950), var(--color-surface-50)) 5%, transparent);
color: var(--color-surface-contrast-950-50); /* <--- add this */
}This will not only fix the combobox issue, but should ensure the preset works as expected in many different use cases. If you could please, remove the combobox style change, add the change to the preset, and update your changeset. That would be great. In this case the changeset will need two changes:
- Switch from
skeleton-commontoskeleton - Update the description to describe the change to the preset
Please and thanks!
Linked Issue
Closes #4239
Description
This addresses issue #4239 where selected items in the ComboBox were
hard to read when reopening the dropdown.
AI Disclosure
Use of LLM technology is allowed. We ask for your voluntary disclosure to help inform future Skeleton contribution guidelines.
Checklist
Please read and apply all contribution requirements.
docs/,feature/,task/,bugfix/mainbranchpnpm checkin the root of the monorepopnpm formatin the root of the monorepopnpm lintin the root of the monorepopnpm testin the root of the monorepo/packageprojects, please supply a ChangesetChangesets
View our documentation to learn more about Changesets. To create a Changeset:
pnpm changesetand follow the prompts