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

ux-select #19

Closed
fkleuver opened this issue Nov 22, 2016 · 39 comments
Closed

ux-select #19

fkleuver opened this issue Nov 22, 2016 · 39 comments

Comments

@fkleuver
Copy link
Member

fkleuver commented Nov 22, 2016

I worked with both Aurelia and Materialize for a while now, and I'd love to help out with a component or two.

Recently I've made a sort of dropdownlist/autocomplete combo in Aurelia.

I'm doing something similar to what Materialize.js is doing, which discards the select and builds a custom list from scratch.

It responds to different UI events like arrow up/down (or mouse wheel) for selecting items, enter to pick an item, esc to close it, and simply typing in it in order to filter the list.

It may be doing too much for a single usercontrol, I just had fun creating it with Aurelia.
I'd like to try and make it fit in with the rest of the ux components.
Example: https://github.com/fkleuver/aurelia-starter/tree/master/src/resources/elements/dropdownlist

I'm thinking about "cutting it in half", making a normal select and a normal autocomplete component out of it.
I didn't have the ux project in mind when I originally created it, so I will still need to make it look like this but I don't expect much trouble there: http://materializecss.com/dropdown.html

Would something like this be functionally okay or is it preferable to stick to the native html select element?
Any thoughts on which direction I should take this?

@EisenbergEffect
Copy link
Contributor

EisenbergEffect commented Nov 23, 2016

We would really like to have au-select :) For a first pass, I wouldn't blend in anything related to auto-complete. I'd just work on the core component.

One thing we'd love to be able to do is have the markup work like a normal select. So, ux-select would have ux-option elements inside them. Inside of each ux-option element would be the template for rendering out that option. Then you'd also be able to use repeat.for to generate options as well.

If you are interested in working on this, then take a crack at it and submit a PR. We can work with you to get it into its final form.

Let me us know!

@fkleuver
Copy link
Member Author

Sounds like a plan :)

How about unit tests though? I'd like to unit test it but I don't see any existing tests in the ux project.

@EisenbergEffect
Copy link
Contributor

We don't have any yet. I'd like to have some. We have our testing library to help out with that. There may be some challenges with using it with the new style system. I'm not sure. If you encounter those, let me know so we can look into it.

@fkleuver
Copy link
Member Author

fkleuver commented Nov 23, 2016

Fair enough, I'll give it a shot. Do you have any objections against me adding a wallaby.js configuration? I'll try to make it work with karma also, I just prefer wallaby myself.

I'm working on it here: https://github.com/fkleuver/ux/tree/ux-select
The unit test works with wallaby.js, but not yet with karma. I can remove wallaby.js again later if you prefer, it just makes it easier for me to get started for now.

@fkleuver fkleuver changed the title ux-select / ux-autocomplete ux-select Nov 23, 2016
@EisenbergEffect
Copy link
Contributor

No objections :)

@fkleuver
Copy link
Member Author

It's not really possible to style a select element without resorting to things like this to disable the default style:

-webkit-appearance: none;
-moz-appearance: none;
appearance: none;

Which is not supported by all browser/versions, I believe. Do you guys have any guidelines with respect to minimum browser support?

Would it otherwise be an idea to have an opt-in browser-default mode like materializecss and then use a list for more customizations?

@Thanood
Copy link

Thanood commented Nov 24, 2016

JFI I'd like to note that MDL (v2) also provides an optional native select implementation.
IMO it's good practice to provide a native select, f.i. for mobile browsers.

@shannonhorn
Copy link

agreed. if there are other minimal browser requirements documented, rob can direct us to those.

@fkleuver
Copy link
Member Author

@Thanood I am currently using the select from http://materializecss.com/forms.html as a basis. It has a custom list by default and the option to use native select. Guess I'll stick to that for now?

@Thanood
Copy link

Thanood commented Nov 24, 2016

@fkleuver I'm indifferent about the Materialize approach since you basically need a css class (which is btw. working more as an identifier) to switch to basic default behaviour. Maybe there are better ways to achieve this.

My personal ideal would be, when using the native version, just a css class which styles the native select to look a bit more like the current theme (maybe an attribute which adds some features).
If you want to use the "real thing" (i.e. all features, completely themed dropdown list, keybindings etc) then add additional elements like material_select does.

So, the other way around, sort of. 😃

@fkleuver
Copy link
Member Author

@Thanood Yea that makes sense, it also keeps the first iteration simpler. Thanks for your suggestion :)

@EisenbergEffect
Copy link
Contributor

One important feature is that we want to be able to provide an arbitrary template for the items in the list. That's something that can't be done with a native select. For accessibility we may need to have a hidden select internally which we populate with text versions of the templated items somehow.

@fkleuver
Copy link
Member Author

@EisenbergEffect Regarding the unit tests, I get this error when trying to configure the plugin from within unit tests:

  ​​​​​%cUnhandled rejection Error: BindingLanguage must implement inspectTextContent().​​​​​
  ​​​​​    at mi (http://localhost:56477/node_modules/aurelia-templating/dist/amd/aurelia-templating.js:763:11)​​​​​
  ​​​​​    at BindingLanguage.inspectTextContent (http://localhost:56477/node_modules/aurelia-templating/dist/amd/aurelia-templating.js:780:7)​​​​​
  ​​​​​    at StyleCompiler.compile (http://localhost:56477/src/styles/style-compiler.js?1480012490036&wallabyFileId=36:42:66)​​​​​
  ​​​​​    at http://localhost:56477/src/styles/style-strategy.js?1480012490207&wallabyFileId=42:120:47​​​​​

If I skip plugin configuration altogether, pretty much everything works fine except the style bindings themselves. The generated classes are appended to document.head but they do not end up in the element's classList.

Can this be related to the different versions in package.json?

@fkleuver
Copy link
Member Author

@EisenbergEffect Regarding the template for the items in the list, I thought about doing this with a wrapped slot element and then injecting its innerHTML into the item template. But I can't just set the innerHTML property of the item template, it would need to be compiled also.

I've worked around this before by wrapping the raw html in <template></template> and passing that to an InlineViewStrategy, then binding that to the view property of compose in the item template. But I've heard someone say that compose has relatively poor performance, is this still true? Would a more low-level approach be preferable in this case?

@Thanood
Copy link

Thanood commented Nov 24, 2016

Maybe with viewCompiler and viewSlots. I've done this for a tree-view. It's not the best or anything but works and afaik it performs well.

@EisenbergEffect
Copy link
Contributor

EisenbergEffect commented Nov 24, 2016 via email

@fkleuver
Copy link
Member Author

fkleuver commented Nov 24, 2016

I made this one before: https://github.com/fkleuver/aurelia-starter/blob/master/src/resources/elements/dynamic-html.ts

Could use that one as-is like so: <dynamic-html html.bind="html" /> but it feels like that wouldn't be much better than just using compose.

If it is (idk about the overhead of having to compile a custom element that encapsulates such logic), though, then wouldn't it be more useful to just add a component like that to templating-resources so it can be reused within the library and by other people? I see people asking for something like this on SO quite frequently. I also use it myself alot.

EDIT
I've made it into a ux-option custom element which seems to work nicely. I'll keep you posted :)

@jdanyow
Copy link
Contributor

jdanyow commented Nov 26, 2016

here's a pure Aurelia autocomplete with the accessibility stuff working in case it helps with building the select element:

https://gist.run/?id=acf8253329939b2e046cd0e3394351fe

@fkleuver
Copy link
Member Author

That will certainly help, thanks!

@fkleuver
Copy link
Member Author

fkleuver commented Nov 28, 2016

Here's a demo of the last commit: http://fkleuver.azurewebsites.net/#/selects
ux: https://github.com/fkleuver/ux/tree/ux-select
app-ux-showcase: https://github.com/fkleuver/app-ux-showcase/tree/ux-select

What works

  • Native and a custom mode
  • Custom options matcher
  • Specify which option property to use for display
  • Custom option template (needs to be escaped, for now)

What's still missing

  • Specify default option
  • Multi select
  • Option groups
  • Specify validation rules
  • Nicer and more compatible styling
  • Effects
  • Theme variables
  • A bunch of unit tests
  • ??

Any feedback is appreciated :)

@EisenbergEffect
Copy link
Contributor

I'd like to see a markup like this work so it's closer to the native:

<ux-select>
  <ux-option repeat.for="item of items" model.bind="item">
    <h5>${item.someProperty}</h5>
    <div>${item.anotherProperty}</div>
  </ux-option>
</ux-select>

@shannonhorn
Copy link

I like that concept too. We've worked extensively with Select2, Selectize, etc. Most client apps require additional information / markup for each option and this would accommodate that.

@fkleuver
Copy link
Member Author

I agree. I'm thinking I should get rid of the intermediate collection thing altogether, because it doesn't really make things easier the way I hoped it would. So having no default markup for the options (you always have to specify it) is OK?

@jdanyow
Copy link
Contributor

jdanyow commented Nov 28, 2016

I do see some benefit with aligning the markup with the native select element but doing so sends us in a direction where we have to deal with synchronization with the repeat that will inevitably be used to render the select options. It also takes away our control over virtualization which will be huge in reducing the number of DOM elements (especially when there are multiple ux-selects on a page and they have complex item templates). I'm not sure yet what the pros/cons will be in terms of accessibility.

I think I still prefer the "ItemsSource" approach where you bind the "items" data and optionally specify the template part for rendering the individual items.

@fkleuver
Copy link
Member Author

I have tried the native approach, but it creates other challenges. I could try to tackle those but I don't know if that will result in a "good" solution.

An example, given this markup:

<ux-select value.bind="value" native-mode.bind=false>
    <ux-option repeat.for="option of options" model.bind="option">
        <h5>${option.text}</h5>
    </ux-option>
</ux-select>

You get this result (removed attributes & comments for readability):

<ux-select value.bind="value" native-mode.bind="false">
    <div>
        <label>
            Option 1
        </label>
        <div>
            <div class="caret"></div>
        </div>
    </div>
    <ul>
        <dynamic-html html.bind="itemTemplateHTML">
            <ux-option model.bind="option">
                <h5>Option 1</h5>
            </ux-option>
            <ux-option model.bind="option">
                <h5>Option 2</h5>
            </ux-option>
            <ux-option model.bind="option">
                <h5>Option 3</h5>
            </ux-option>
        </dynamic-html>
    </ul>
    <div ref="itemTemplate" show.bind="false">
        <ux-option model.bind="option">
            <h5>Option 1</h5>
          </ux-option>
          <ux-option model.bind="option">
            <h5>Option 2</h5>
          </ux-option>
          <ux-option model.bind="option">
            <h5>Option 3</h5>
          </ux-option>
    </div>
</ux-select>

ux-option won't receive the right bindingContext this way. The itemTemplate (including the repeat) is already compiled in the declaring context and then it's passed down to ux-select as raw HTML, where it is compiled again. The second compilation results in option being undefined, as is to be expected. I guess I could move itemTemplate.childNodes to ulElement (because setting innerHTML kills the behavior of ux-option) but then that needs to be synchronized on any change in the options collection, so we would need the "itemsSource" either way I think.

@fkleuver
Copy link
Member Author

fkleuver commented Nov 28, 2016

@EisenbergEffect I think I could do it your way if there is a reliable method to retain the bindingContext in which the items are declared, or merge it into ux-select's bindingContext. Then an option would be to add a processContent decorator to ux-select and skip compiling the itemTemplate, so ux-select itself can take care of compiling the item template with repeat.for and all. Does that make sense?

@fkleuver
Copy link
Member Author

fkleuver commented Dec 1, 2016

@EisenbergEffect @jdanyow The more I think about it, the more I'm leaning towards a "html generator" approach. In other words: collect the information from the bindables and the slot's innerHTML (even allowing the user to set the raw HTML property directly, if they wish), and then just programmatically build up the right string of html and pass that to the compiler. The added flexibility opens up all sorts of possibilities and I don't see any issues with supporting the usage methods mentioned so far.

Are there any factors I'm overseeing that make this a no-no, or shall I give it a shot?

@EisenbergEffect
Copy link
Contributor

I'm open to anything just about. We can experiment and see what we think works best. Are you aware of the @processContent(...) hook?

@jdanyow
Copy link
Contributor

jdanyow commented Dec 1, 2016

The one concern I have is the impact of rendering all the options out to the DOM, especially when the item-templates are complex or there are a lot of items. Off the cuff I'd bet on an approach that uses the template-part functionality and only renders the 10 or 15 options that are visible in the drop-down's view-port, only when the drop-down is open. This will be a potentially massive reduction in dom elements depending on the complexity of the template, number of options and number of selects on the form. There's an example of this here- minus the logic to only render the visible options.

@EisenbergEffect
Copy link
Contributor

That's a good argument and it definitely supports the notion of having an items-source. Let's go with that. We don't necessarily need to use template parts though either. We could assume that the content of the ux-select is the template. Then we could use the processContent hook to extract it and compiler it into a view factory. It could even be auto-wrapped in a container element as part of that if desired.

@khuongduybui
Copy link
Contributor

any update?

@fkleuver
Copy link
Member Author

fkleuver commented Aug 8, 2017

Apologies for not following up, I've been very busy the past 8 months. I will continue working on this in the coming weeks.

@serifine serifine closed this as completed Sep 1, 2017
@serifine
Copy link
Contributor

serifine commented Sep 1, 2017

Issue moved to aurelia/ux-components #15 via ZenHub

@serifine serifine reopened this Oct 6, 2017
@bigopon
Copy link
Member

bigopon commented Dec 9, 2017

From my experience, drop-down list needs to be rendered to body, not the component element. Rendering list to the element gives pleasantly easy styling / positioning but will not play well within a scroll element, or placed inside overflow hidden container path.

Edit: Actually using position: fixed will help. I have always position absolute to avoid mobile issue but it shouldn't be a problem for ux targets

@fkleuver Let me know if you need any help continuing the element

@serifine
Copy link
Contributor

@bigopon is going to take over development for this. It is a pretty critical piece.

@serifine
Copy link
Contributor

Adding this to the 0.7.0 Milestone

@serifine serifine added this to the 0.7.0 milestone Dec 20, 2017
@bigopon
Copy link
Member

bigopon commented Dec 21, 2017

For accessibility, there are two options to implement select list:

role="menu"

This seems to be common implementation.

role="listbox"

Even the homepage of material design doesn't have a specific section for select ? page.

listbox seems to be suitable for ux-select. On the other hand, Doing menu gives us advantages of sharing code between ux-menu and ux-select. But I think it's ok to separate them for ux. The example from @jdanyow also used listbox

@EisenbergEffect @ZHollingshead listbox for ux-select and menu for ux-menu or I should implement both ux-menu and ux-select ? I'm leaning towards listbox.

@serifine
Copy link
Contributor

I would think that it would depend.

If the user is selecting only one item at a time, i.e. a standard select, menu.

If the user is allowed to select multiple attributes, i.e. <ux-select multiple> for a multi select, then I would think listbox becomes more appropriate.

If we choose only one, listbox would be more appropriate in my opinion because it explicitly states that it allows the user to select one or more items.

@EisenbergEffect
Copy link
Contributor

@bigopon It's worth looking here: https://github.com/EisenbergEffect/aurelia-composition-demo/tree/master/src/part-composition There are some ideas there on how to use parts for list item and selected item, including syntax enhancements and APIs for simple and advanced scenarios. It might provide a good staring point. It would need quite a bit of CSS, accessibility, etc. work though.

@serifine serifine modified the milestones: 0.7.0, 0.8.0 Jan 3, 2018
@serifine serifine removed this from the 0.8.0 milestone Mar 8, 2018
@serifine serifine closed this as completed Nov 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Aurelia UX
Awaiting triage
Development

No branches or pull requests

8 participants