-
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 <175728472+Copilot@users.noreply.github.com>
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 how the first <option>
placeholder is handled by removing the hidden
attribute (which had inconsistent behavior across browsers) and instead using a .placeholder
CSS class to control styling and visibility. It conditionally renders this placeholder option in the component, updates related selectors in SCSS, and refreshes all snapshots to match the new behavior.
- Replace
[hidden]
filtering in CSS with.placeholder
- Render placeholder option only when needed and update fallback logic
- Refresh snapshots to reflect the first option no longer being hidden
Reviewed Changes
Copilot reviewed 14 out of 18 changed files in this pull request and generated 4 comments.
Show a summary per file
File | Description |
---|---|
showcases/screen-reader/snapshots/windows/chromium/DBSelect-default-1.txt | Updated screen reader snapshot to reflect placeholder option behavior |
packages/components/src/styles/internal/_form-components.scss | Switched :not([hidden]) to :not(.placeholder) in focus selector |
packages/components/src/components/select/select.scss | Updated :not([hidden]) filter to .placeholder class |
packages/components/src/components/select/select.lite.tsx | Conditionally render placeholder <option> and changed fallback logic |
packages/components/src/components/select/index.html | Updated example to use .placeholder class for first <option> |
snapshots/select/** | Updated ARIA snapshots across browsers to match new option behavior |
@@ -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.
Using ||
here will fall back to props.label
when props.placeholder
is an empty string; consider using the nullish coalescing operator (??
) to only fall back on null
or undefined
.
{props.placeholder || props.label} | |
{props.placeholder ?? props.label} |
Copilot uses AI. Check for mistakes.
@@ -229,7 +229,9 @@ export default function DBSelect(props: DBSelectProps) { | |||
} | |||
aria-describedby={props.ariaDescribedBy ?? state._descByIds}> | |||
{/* Empty option for floating label */} | |||
<option hidden></option> | |||
<Show when={props.variant === 'floating' || props.placeholder}> | |||
<option class="placeholder" value=""></option> |
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 placeholder <option>
has no visible text; screen readers may announce it as blank. Add inner text or an aria-label
matching the placeholder so users understand its purpose.
<option class="placeholder" value=""></option> | |
<option class="placeholder" value="">{props.placeholder}</option> |
Copilot uses AI. Check for mistakes.
@@ -9,7 +9,7 @@ | |||
<div class="db-select"> | |||
<label for="test1">Label</label> | |||
<select id="test1" aria-describedby="db-infotext-01"> | |||
<option hidden></option> | |||
<option class="placeholder" value=""></option> |
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 example’s placeholder option is empty and lacks accessible text. Include placeholder text or an aria-label
so it's announced properly in the dropdown.
<option class="placeholder" value=""></option> | |
<option class="placeholder" value="">Select an option</option> |
Copilot uses AI. Check for mistakes.
@@ -18,7 +18,7 @@ | |||
<div class="db-select"> | |||
<label for="test2">Label</label> | |||
<select id="test2" aria-describedby="db-infotext-02"> | |||
<option hidden></option> | |||
<option class="placeholder" value=""></option> |
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.
This second example also uses an empty placeholder <option>
; please add visible or ARIA text to ensure it’s clear when navigating the list.
<option class="placeholder" value=""></option> | |
<option class="placeholder" value="">Select an option</option> |
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