Skip to content
This repository has been archived by the owner on Dec 8, 2022. It is now read-only.

Contrib > Select field component #1629

Merged
merged 59 commits into from
Apr 23, 2018
Merged

Conversation

Blackbaud-SteveBrush
Copy link
Member

Original contribution: #1496

Addresses: #155

@codecov-io
Copy link

codecov-io commented Apr 14, 2018

Codecov Report

Merging #1629 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1629      +/-   ##
==========================================
+ Coverage   99.98%   99.98%   +<.01%     
==========================================
  Files         391      395       +4     
  Lines        7889     8069     +180     
  Branches     1152     1183      +31     
==========================================
+ Hits         7888     8068     +180     
  Misses          1        1
Impacted Files Coverage Δ
...ules/select-field/select-field-picker.component.ts 100% <100%> (ø)
src/modules/select-field/select-field.module.ts 100% <100%> (ø)
...odules/select-field/select-field-picker-context.ts 100% <100%> (ø)
src/modules/select-field/select-field.component.ts 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 049e29f...ada9887. Read the comment docs.

Copy link

@Blackbaud-BobbyEarl Blackbaud-BobbyEarl left a comment

Choose a reason for hiding this comment

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

In the reactive form demo, after selecting a favorite color, I get different behavior when clicking the "x" versus tabbing to it then hitting enter.

The disabled select field is still accessible via the keyboard.

In the select field, I like that the "x" is accessible via the keyboard, this is different than the behavior in the sky-list. Just want to make sure that's expected.

ListItemModel
} from '../list/state';

import { SkyListFilterInlineModel } from '../list-filters/list-filter-inline.model';

Choose a reason for hiding this comment

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

I personally like all imports on a separate line, but am OK using a single line if it's one import. In any case, I think we should just be consistent. Down below there are more examples of both implementations.

Copy link
Member Author

Choose a reason for hiding this comment

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

For new components I've definitely tried to be consistent with single-file imports vs multi-line barrel imports. Happy to look at some instances where I haven't been consistent, however.

Copy link
Member Author

Choose a reason for hiding this comment

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

FYI: #1647

Copy link
Contributor

Choose a reason for hiding this comment

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

The clear X should be accessible via keyboard in this case. I believe the reason we don't do it e.g. in Search is that the user can clear the text via backspace/delete, which is not the case with Select Field

@Blackbaud-SteveBrush
Copy link
Member Author

Blackbaud-SteveBrush commented Apr 20, 2018

@Blackbaud-BobbyEarl Good eye catching the close button and tabindex issues; those have been addressed. I reached out to @Blackbaud-ToddRoberts to provide some insight around the tab-able clear button on the single-select search field.

@Blackbaud-BobbyEarl Blackbaud-BobbyEarl merged commit 0774b26 into master Apr 23, 2018
@Blackbaud-BobbyEarl Blackbaud-BobbyEarl deleted the contrib-select-field branch April 23, 2018 13:37
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants