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.

Sign up for GitHub

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

fix(listbox): use filter tag #5952

Closed
wants to merge 17 commits into from
Closed

fix(listbox): use filter tag #5952

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
0dd3e5e
fix(listbox): use filter tag
janhassel Apr 28, 2020
8e8bd9c
fix(listbox): restore onKeyDown handler
janhassel Apr 28, 2020
dd1c100
fix(listbox): fix tag spacing on filterable multiselect
janhassel Apr 30, 2020
2327f5f
fix(listbox): remove explicit aria-label from tag
janhassel Apr 30, 2020
2718b37
Merge branch 'master' into listbox-tag
janhassel Apr 30, 2020
d859ed4
test(listbox): update snapshots
janhassel Apr 30, 2020
81d51e5
test(listbox): update snapshots
janhassel Apr 30, 2020
66270e5
test(listbox): update selectors
janhassel Apr 30, 2020
817907f
fix(listbox): apply correct styling to disabled tag
janhassel Apr 30, 2020
b7c779c
chore(listbox): fix prettier issue
janhassel Apr 30, 2020
481e914
fix(listbox): pass disabled state to tag
janhassel May 1, 2020
dcb9f62
Merge branch 'master' into listbox-tag
janhassel May 4, 2020
5dd96db
Merge branch 'master' into listbox-tag
janhassel May 18, 2020
e90ccfa
fix(multi-select): correct clear input button for filterable multiselect
janhassel May 31, 2020
5400128
Merge branch 'master' into listbox-tag
janhassel May 31, 2020
abc0b98
chore(multi-select): fix prettier issue
janhassel May 31, 2020
d62061a
Merge branch 'master' into listbox-tag
laurenmrice Jun 1, 2020
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
27 changes: 3 additions & 24 deletions packages/components/src/components/list-box/_list-box.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
@import '../../globals/scss/helper-mixins';
@import '../../globals/scss/layout';
@import '../../globals/scss/vendor/@carbon/elements/scss/import-once/import-once';
@import '../tag/tag';
@import '../../globals/scss/css--reset';

/// @type Number
Expand Down Expand Up @@ -388,42 +389,20 @@ $list-box-menu-width: rem(300px);
}

// Modifier for a selection to show that multiple selections have been made
.#{$prefix}--list-box__selection--multi {
@include type-style('label-01');
.#{$prefix}--tag.#{$prefix}--list-box__selection--multi {
position: static;
display: flex;
align-items: center;
justify-content: space-between;
padding: 0;
background-color: $inverse-02;
height: rem(24px);
width: auto;
color: $inverse-01;
line-height: 0;
padding: rem(8px);
padding-right: rem(2px); // Align with hover circle of X button
margin-right: rem(10px);
margin-left: 0;
border-radius: rem(12px);
}

.#{$prefix}--list-box__selection--multi > svg {
fill: $inverse-01;
margin-left: rem(4px);
width: rem(20px);
height: rem(20px);
padding: rem(2px);
}

.#{$prefix}--list-box__selection--multi > svg:hover {
border-radius: 50%;
background-color: $hover-secondary;
}

.#{$prefix}--list-box__selection--multi:focus,
.#{$prefix}--list-box__selection--multi:hover {
outline: none;
}

// Descendant of a `list-box` that displays a list of options to select
.#{$prefix}--list-box__menu {
@include box-shadow();
Expand Down
13 changes: 6 additions & 7 deletions packages/react/src/components/ListBox/ListBoxSelection.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,9 @@
import cx from 'classnames';
import React from 'react';
import PropTypes from 'prop-types';
import { Close16 } from '@carbon/icons-react';
import { settings } from 'carbon-components';
import { match, keys } from '../../internal/keyboard';
import Tag from '../Tag';

const { prefix } = settings;

Expand All @@ -26,7 +26,6 @@ const ListBoxSelection = ({
disabled,
}) => {
const className = cx(`${prefix}--list-box__selection`, {
[`${prefix}--tag--filter`]: selectionCount,
[`${prefix}--list-box__selection--multi`]: selectionCount,
});
const handleOnClick = event => {
Expand All @@ -49,17 +48,17 @@ const ListBoxSelection = ({
};
const description = selectionCount ? t('clear.all') : t('clear.selection');
return (
<div
role="button"
<Tag
Copy link
Member

Choose a reason for hiding this comment

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

I think we can add the disabled prop here so we can get the correct styles when the multiselect is disabled

Copy link
Member Author

Choose a reason for hiding this comment

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

I was thinking that too and compared it to the current implementation where the tag styling is not changed on disable.

Personally I would also prefer to have the tag disabled since otherwise it really stands out. Do you if there was a reason behind the current implementation, like keeping it well readable for accessibility maybe?

Copy link
Member

Choose a reason for hiding this comment

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

it looks like there is a slight color change on the disabled tag svg in the current multiselect, but I'm not sure if it should look like the standalone disabled tag styles cc @laurenmrice

Copy link
Member Author

Choose a reason for hiding this comment

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

Just as a reference: this is how it would look when the tag enters disabled state as well:
image

Copy link
Member

@laurenmrice laurenmrice Apr 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Let me make a spec for this 👍🏻, will get back to you shortly

Copy link
Member

Choose a reason for hiding this comment

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

This is what we can do for a selected-disabled state.

tags

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the quick specs, @laurenmrice 👍
I updated the tag color for the disabled state accordingly, you can check it out in the deploy preview: https://5eaae7cd3ab6150007c0340b--carbon-components-react.netlify.app/?path=/story/multiselect--default

className={className}
tabIndex={disabled ? -1 : 0}
type="high-contrast"
filter
onClose={handleOnClick}
onClick={handleOnClick}
onKeyDown={handleOnKeyDown}
aria-label="Clear Selection"
title={description}>
{selectionCount}
<Close16 />
</div>
</Tag>
);
};

Expand Down