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

Hrobbins/calcite select #2 #44

Closed
wants to merge 6 commits into from
Closed

Conversation

pr3tori4n
Copy link
Contributor

@pr3tori4n pr3tori4n commented Jun 6, 2019

Relates to Issue #2

This adds a component which takes tags as children.
By default it renders a native select dropdown.
It accepts an enhanced attribute || prop which changes it to use a hidden input and other dom nodes for enhanced styling.

TODO:

  1. Tests - It has a test but its basic, there's a few other things that could be tested, such as the name attribute, the selected attribute on one of the options, and the enhanced version.
  2. Styling - I copied the styling from Calcite-web v2 for a Select tag and the dropdown. I believe there is more to be done, but that's unclear without getting design review.

I'm putting work on this on hold for now to switch gears to work on components needed in calcite-app-components.

if(!this.enhanced) {
return (
<Host>
<select {...this.hostAttributes}>
Copy link
Contributor Author

@pr3tori4n pr3tori4n Jun 6, 2019

Choose a reason for hiding this comment

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

Passing attributes through is something I'm not sure how best to go about. It seems reasonable that a dev-user of this component could expect any valid html attributes they pass in to reflect on the select tag. But we could also white-list the attributes we expect to be important (name is a must for forms to work). I fear we may omit something crucial to a given use-case if we do that though.

@patrickarlt patrickarlt mentioned this pull request Jun 9, 2019
Copy link
Contributor

@patrickarlt patrickarlt left a comment

Choose a reason for hiding this comment

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

@pr3tori4n I feel like <calcite-select> might suffer from a bit of the same fate as <calcite-switch> where we didn't have a clear enough idea of what we wanted or how we wanted to do things before development started. I added a bunch of comments in #2 the help clarify what this should do.

If you have time to tackle new implementation that is great, but if you don't I feel like it might be best to close this PR without merging it and note in the select issue that a basic select component was started and someone can pick it up from your branch if they want.

@patrickarlt
Copy link
Contributor

Closing after discussion in #2.

@paulcpederson paulcpederson deleted the hrobbins/calcite-select-#2 branch March 11, 2020 21:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants