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

adding autocomplete support to bunsen #522

Merged
merged 8 commits into from
May 2, 2018

Conversation

helrac
Copy link
Contributor

@helrac helrac commented Apr 30, 2018

Overview

Summary

This implemented the autocomplete renderer into bunsen. Autocomplete has also been added into the dummy app.

screen shot 2018-04-30 at 4 00 29 pm

Screenshots or recordings

Please provide screenshots or recordings if this PR is modifying the visual UI or UX.

Semver

  • #none#
  • #patch#
  • #minor#
  • #major#

CHANGELOG

  • Added autocomplete renderer to bunsen (interchangeable with select)

@helrac helrac requested review from a team as code owners April 30, 2018 20:00
@@ -0,0 +1,259 @@
## autocomplete

This renderer provides an autocomplete input which allows one option to be selected.
Copy link
Contributor

Choose a reason for hiding this comment

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

Expand on he differences between select and autocomplete please.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, the rest of the docs is a dup to select. I'd hate to have to maintain 2 separate docs if the implementation for one is altered. Perhaps provide a link and mention that the API is the same as select.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ya, i'll reference select. I didn't like duping it anyways

export default {
properties: {
cookies: {
enum: [
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe using an enum isn't the best choice for demo. Since you're trying to show that autocomplete is a little different from select. Using an enum the value is validated again the enum. For autocomplete I believe free text is allowed which would fail validation if that text wasn't inenum.

Copy link
Contributor

Choose a reason for hiding this comment

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

In fact, you might want to provide some protection against using enum for autocomplete. Do you agree?

Copy link
Contributor Author

@helrac helrac May 1, 2018

Choose a reason for hiding this comment

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

Well, it's just a different style of dropdown, i see no reason to block using enum. It's up to the end user. The example under examples for autocomplete/autocomplete uses a query. I could switch the examples above to use a query.

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 just style that's different though. With select you have to choose from the list of available options. With autocomplete you don't have to. This means the value can optionally come from the dropdown list and that behavior breaks the meaning of enum.

So I can understand not wanting to alter the behavior to intentionally block enum but at the very least, lets not encourage people to use it with enum.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, in the documentation I will list what scenario is ideal for autocomplete. IE, wanting the user to type to search for options

*/
filterOptions (filter) {
const value = this.get('formValue')
run.debounce(this, this.runUpdateItems, {value, filter, keepCurrentValue: false}, DEBOUNCE_INTERVAL)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does the autocomplete get the debounce but select doesn't?

Copy link
Contributor Author

@helrac helrac May 1, 2018

Choose a reason for hiding this comment

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

Well, I didn't want to change the functionality of select, I could give it to select but give it a debounce of 0 unless otherwise specified if you'd like

Copy link
Contributor

@sophypal sophypal left a comment

Choose a reason for hiding this comment

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

Looks good but some minor concerns with the enum behavior.

@@ -551,6 +551,7 @@ export default AbstractInput.extend({
*/
filterOptions (filter) {
const value = this.get('formValue')
console.log('filtering for ' + filter)
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove

sophypal
sophypal previously approved these changes May 1, 2018
@@ -952,14 +953,14 @@
},
"autoprefixer": {
"version": "7.2.6",
"resolved": "https://registry.npmjs.org/autoprefixer/-/autoprefixer-7.2.6.tgz",
"integrity": "sha512-Iq8TRIB+/9eQ8rbGhcP7ct5cYb/3qjNYAR2SnzLCEcwF6rvVOax8+9+fccgXk4bEhQGjOZd5TLhsksmAdsbGqQ==",
"resolved": "https://artifactory.ciena.com/api/npm/blueplanet-npm/autoprefixer/-/autoprefixer-7.2.6.tgz?dl=https://registry.npmjs.org/autoprefixer/-/autoprefixer-7.2.6.tgz",
Copy link
Contributor

Choose a reason for hiding this comment

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

This is incorrect. Since this is a public repo you do not want it to point at artifactory. To fix this you will need to remove the setting in your dev environment that is setting the registery to artifactory for everything and instead place a .npmrc file with that registry setting in your directories for private repos only.

package.json Outdated
@@ -87,7 +87,7 @@
"ember-cli-svgstore": "github:ciena-blueplanet/ember-cli-svgstore#977df1cf58ae43b1d98a591573c3e06947744321",
"ember-computed-decorators": "0.3.0",
"ember-concurrency": "0.7.19",
"ember-frost-core": "^8.0.0",
"ember-frost-core": "^8.2.2",
Copy link
Contributor

Choose a reason for hiding this comment

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

This change should be reverted. It is not necessary to update the version of ember-frost-core here since it will already pick up the minor due to the ^ float.

@helrac helrac merged commit 56e1916 into ciena-frost:master May 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants