Skip to content
This repository was archived by the owner on Oct 2, 2019. It is now read-only.

Review: Add choices grouping using 'group by' expression #99

Closed
wants to merge 3 commits into from

Conversation

dimirc
Copy link
Contributor

@dimirc dimirc commented Jul 10, 2014

WIP

This PR is based on #49 but with some changes based on the comment I left there.

Basically navigation with up/down key is fixed in "grouped mode"

TODO:

  • Some tests are failing
  • Make a demo with iterator function
  • Choices template needs to show correctly only simple/grouped block
  • Check correct format of all themes
  • Update README with demo of this feature

Demo plunker

cc: @just-boris

@dimirc dimirc added this to the 0.4.0 milestone Jul 10, 2014
@dimirc dimirc changed the title Review: Add choices grouping Add choices grouping Jul 10, 2014
@dimirc dimirc changed the title Add choices grouping Review: Add choices grouping Jul 10, 2014
@dimirc
Copy link
Contributor Author

dimirc commented Jul 11, 2014

@just-boris I rebase with last changes of your #49 and did some minor changes, basically:

  • Add a new demo-groupby.html to show demo of different grouping options. Demo plunker
  • Tweak _ensureHighlightVisible() to make group header visible when going all the way up with arrow key
  • Separate the way simple/grouped element is shown at choices template. (since we couldn't transparently use ngIf at template and be able to get the element at linkFn, I did some element manipulation using their class names)

There are some new failing tests that should be checked. I guess they appear because of the changes I did at the choices template

@dimirc dimirc changed the title Review: Add choices grouping Review: Add choices grouping using 'group by' expression Jul 11, 2014
@just-boris
Copy link
Contributor

@dimirc you have done a good job!
I've tried to fix a tests, you are right, changes in templates make a deal. Should I update my pull-request with recent fixes, or you do the rest of work yourself?

@dimirc dimirc closed this Jul 11, 2014
@dimirc dimirc deleted the just-boris-master branch July 11, 2014 21:53
@dimirc
Copy link
Contributor Author

dimirc commented Jul 11, 2014

The changes at the templates to separate simple/grouped blocks will involve too much effort and additional code that might not be worth (select2 theme was working ok, but the 2 other themes needed some additional fixes)

We'll continue with the approach from #49 and with some extra changes at #102

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.

2 participants