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

feat: select component a11y changes #1066

Merged
merged 21 commits into from
Jun 9, 2020
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
259 changes: 147 additions & 112 deletions docs/pages/components/select.md

Large diffs are not rendered by default.

Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
3 changes: 1 addition & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -123,8 +123,7 @@
"pre-commit": {
"colors": true,
"run": [
"lint:pre-commit",
"storybook:visual-docker"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This shouldn't be running storybook on a pre-commit hook

Copy link
Contributor

Choose a reason for hiding this comment

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

This should be moved to separate PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It already was - otherwise you wouldn't be able to commit anything 😉
#1067

"lint:pre-commit"
],
"silent": false
},
Expand Down
1 change: 1 addition & 0 deletions src/mixins/_forms.scss
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@
}

&[readonly],
&[aria-readonly="true"],
Copy link
Contributor

Choose a reason for hiding this comment

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

Should use @mixin fd-readonly

&.is-readonly {
background-color: rgba(242, 242, 242, 0.5); // Hardcoded for IE11 Purposes, there is not callback for RGBA.
background-color: var(--sapField_ReadOnly_Background);
Expand Down
10 changes: 7 additions & 3 deletions src/select.scss
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ $block: #{$fd-namespace}-select;

display: block;
padding-right: 0;
min-width: 5rem;

&__control {
@include fd-form-base();
Expand Down Expand Up @@ -53,7 +54,8 @@ $block: #{$fd-namespace}-select;
}
}

&.is-expanded {
&.is-expanded,
&[aria-expanded="true"] {
.#{$block}__button {
background-color: $fd-select-button-active-background;
color: $fd-select-button-active-text-color;
Expand All @@ -75,7 +77,6 @@ $block: #{$fd-namespace}-select;
}
}

&[readonly],
&.is-readonly {
padding-right: $fd-select-padding-x;
padding-left: $fd-select-padding-x;
Expand All @@ -87,6 +88,10 @@ $block: #{$fd-namespace}-select;
font-size: var(--sapFontSize);
}

&__item[aria-selected=true] {
border-bottom: 0 !important;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

It's not fiori3 compatible. First pictures on specifications are wrong.
image
Anyway, we don't use important at all. If this situation occurs, you would need to use stronger selector

Copy link
Contributor

@JKMarkowski JKMarkowski Jun 2, 2020

Choose a reason for hiding this comment

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

So we don't need to have fd-select__item on every single item anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Which spec do we want to follow as the source of truth? https://experience.sap.com/fiori-design-web/select/
Screen Shot 2020-06-02 at 8 05 40 AM

Copy link
Contributor

Choose a reason for hiding this comment

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

I can only assume, that we should follow list component there. Same as with multi input or combobox. Let's ask designers maybe, because spec shows 2 different views.

&--compact {
.#{$block}__control {
height: $fd-form-input-height--compact;
Expand All @@ -98,7 +103,6 @@ $block: #{$fd-namespace}-select;
padding-left: 0;
}

&[readonly],
&.is-readonly {
padding-right: $fd-select-padding-compact-x;
padding-left: $fd-select-padding-compact-x;
Expand Down
78 changes: 78 additions & 0 deletions stories/select/select.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
<script>
import { createEventDispatcher } from "svelte";
const dispatch = createEventDispatcher();

export let select;

function onSelectClick() {
select.openDropdown = !select.openDropdown;
dispatch("onSelectClick", select);
}
</script>

<div dir="{select.dir}">
<label class="fd-form-label" id="f45564A57">
Label examples
</label>
<br />
<div class="fd-popover">
<div
aria-disabled="{select.disabled}"
aria-readonly="{select.readOnly}"
class="fd-popover__control">
<div class="fd-select {select.compact ? 'fd-select--compact' : ''}">
<div
aria-controls="h0C6A325"
aria-disabled="{select.disabled}"
aria-expanded="{select.openDropdown}"
aria-haspopup="true"
class="fd-select__control {select.readOnly ? 'is-readonly' : ''}"
role="button"
tabindex="0"
on:click="{!select.readOnly && onSelectClick}">
{select.placeholder ? select.placeholder : 'Select'}
{#if !select.readOnly}
<span class="fd-button fd-button--transparent sap-icon--slim-arrow-down fd-select__button"></span>
{/if}
</div>
</div>
</div>
<div
aria-hidden="{!select.openDropdown}"
class="fd-popover__body fd-popover__body--no-arrow fd-popover__body--dropdown {select.dropdownFill ? 'fd-popover__body--dropdown-fill' : ''}"
id="h0C6A325">
{#if select.formMessage}
<div
aria-live="assertive"
class="fd-list__message fd-list__message--{select.formMessage.type}"
role="alert">
{select.formMessage.message}
</div>
{/if}
<ul
aria-labelledby="f45564A57"
class="fd-list fd-list--dropdown {select.compact ? 'fd-list--compact' : ''} {select.formMessage ? 'fd-list--has-message' : ''}"
role="listbox"
tabindex="-1">
{#each select.optionData as option}
<li
aria-selected="{option.selected}"
class="fd-list__item fd-select__item"
role="option"
tabindex="-1">
{#if option.icon}
<span class="fd-list__icon {option.icon}"></span>
{/if}
<span class="fd-list__title {option.noWrap ? 'fd-list__title--no-wrap' : ''}">{option.text}</span>
{#if option.secondaryText}
<span class="fd-list__secondary">{option.secondaryText}</span>
{/if}
</li>
{/each}
</ul>
</div>
</div>
{#if select.formMessage && !select.openDropdown}
<span class="fd-form-message fd-form-message--static fd-form-message--{select.formMessage.type}">{select.formMessage.message}</span>
{/if}
</div>
Loading