-
Notifications
You must be signed in to change notification settings - Fork 10
refactor(selector): not hiding the first option
anymore
#4020
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?
Conversation
it's inconsistent in between browsers and we should only use it for placeholders or floating labels
🔭🐙🐈 Test this branch here: https://db-ux-design-system.github.io/core-web/review/4019-dbselect-dropdown-checkmark-on-first-element |
Co-authored-by: Copilot <[email protected]>
option
anymore
option
anymoreoption
anymore
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
…ttps://github.com/db-ux-design-system/core-web into 4019-dbselect-dropdown-checkmark-on-first-element
This reverts commit 49ca700.
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
…ttps://github.com/db-ux-design-system/core-web into 4019-dbselect-dropdown-checkmark-on-first-element
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
…ttps://github.com/db-ux-design-system/core-web into 4019-dbselect-dropdown-checkmark-on-first-element
This reverts commit 88cea12.
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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.
Pull Request Overview
This PR refactors the select component to stop hiding the first option element, addressing browser inconsistencies and improving HTML standards compliance. The change replaces the hidden
attribute approach with a conditional placeholder option that uses a CSS class for identification.
- Removes
hidden
attribute from first option and replaces with conditional placeholder option using.placeholder
class - Updates CSS selectors to target
.placeholder
class instead of[hidden]
attribute - Changes logical operator from
??
to||
for placeholder text fallback
Reviewed Changes
Copilot reviewed 14 out of 18 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
packages/components/src/components/select/select.lite.tsx |
Implements conditional placeholder option rendering and updates placeholder text logic |
packages/components/src/components/select/select.scss |
Updates CSS selector to target .placeholder class instead of [hidden] attribute |
packages/components/src/styles/internal/_form-components.scss |
Updates form component styles to use .placeholder class selector |
packages/components/src/components/select/index.html |
Updates HTML examples to use new placeholder option structure |
Multiple snapshot files | Test snapshots updated to reflect new accessibility tree structure |
@@ -296,7 +298,7 @@ export default function DBSelect(props: DBSelectProps) { | |||
</Show> | |||
</select> | |||
<span id={state._placeholderId}> | |||
{props.placeholder ?? props.label} | |||
{props.placeholder || props.label} |
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.
The change from ??
(nullish coalescing) to ||
(logical OR) alters the behavior for empty strings. The ??
operator only falls back for null/undefined, while ||
also falls back for empty strings, which may be unintended if empty placeholder strings should be preserved.
{props.placeholder || props.label} | |
{props.placeholder ?? props.label} |
Copilot uses AI. Check for mistakes.
Proposed changes
option[hidden]
)select
-HTML-elements, as those would actually mark the firstoption
as checked as well, whereas this is actually the selected one (gets transmitted in form submits etc.)placeholder
, there should be anoption
that the users could set to "unselect" all selectable options, or in other words "reset" any other selection. We wouldn't provide this possibility at the moment anyhow.Resolves #4019
Types of changes
Further comments