Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

Introduced select UI component #530

Closed
wants to merge 9 commits into from
Closed

Introduced select UI component #530

wants to merge 9 commits into from

Conversation

mlewand
Copy link
Contributor

@mlewand mlewand commented Nov 21, 2019

Suggested merge commit message (convention)

Feature: Introduced the SelectView component. Closes ckeditor/ckeditor5#5770.


Additional information

I didn't implement isReadOnly property as readonly is not supported on select elements in HTML so that would require workarounds.

Also I was wondering whether changing value to a value that is not contained by the select should throw an error or silently fail, doing no harm. I went with the latter, though it would be nice to log some information, but I believe we have no common mechanisms for that.

There's a subpr with manual test to this component: ckeditor/ckeditor5-theme-lark#253

@coveralls
Copy link

coveralls commented Nov 21, 2019

Coverage Status

Coverage remained the same at 100.0% when pulling add59d8 on i/5770 into 95404e3 on master.

@mlewand mlewand requested a review from oleq November 22, 2019 11:54
@oleq
Copy link
Member

oleq commented Nov 26, 2019

We went with a DropdownView in ckeditor/ckeditor5#1110 in this iteration to speed things up and use a component that is known to us. Whether we will use this component in the future, it's still unclear to the team and it needs to be discussed. For now, I'm unplugging it from the iteration.

@mlewand
Copy link
Contributor Author

mlewand commented Dec 2, 2019

Closing the PR, see ckeditor/ckeditor5#5770 (comment)

@mlewand mlewand closed this Dec 2, 2019
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.

Introduce select UI component
3 participants