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

remove sorting from splitByGroups #9

Merged
merged 3 commits into from
Oct 9, 2017
Merged

remove sorting from splitByGroups #9

merged 3 commits into from
Oct 9, 2017

Conversation

abmantis
Copy link
Contributor

@abmantis abmantis commented Oct 5, 2017

This is needed for home-assistant/frontend#443

Should I bump the version and update the dependency version on home-assistent-polymer?

lib/group.js Outdated
// Returns { groups: [], ungrouped: {} }
export function splitByGroups(entities) {
export function splitByGroups(entities, sort = true) {
Copy link
Member

Choose a reason for hiding this comment

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

Let's keep this function simple and remove sorting the groups array completely.

Where it is needed, the ordering can be done by the code calling this.

Make sure to update the test too.

@balloob
Copy link
Member

balloob commented Oct 7, 2017

Shit, I just realized that this is not going to work either.

See, now the groups are not ordered at all. Including not being ordered by the view. The problem is that in the polymer-side of things, we pass down an object of entities and then have it use this function to figure out what to do.

However, objects in JavaScript are iterated over in random order instead of insertion order.

@balloob
Copy link
Member

balloob commented Oct 7, 2017

So I guess that we can close this PR and solve this completely in polymer. Instead of passing down isView, we should pass down the order of groups to use (based on the entity ids in the view)

@balloob
Copy link
Member

balloob commented Oct 8, 2017

Closing this as it is no longer needed

@balloob balloob closed this Oct 8, 2017
@abmantis
Copy link
Contributor Author

abmantis commented Oct 8, 2017

I think we should also merge this. There is no need to keep the sorting here since it is done outside.

@balloob balloob reopened this Oct 8, 2017
@balloob
Copy link
Member

balloob commented Oct 8, 2017

Good point. Can you move the sorting into the tests? There is no guarantee that this order is going to be always like this as it's based on Object.keys.

@balloob balloob merged commit f7789a7 into home-assistant:master Oct 9, 2017
@balloob
Copy link
Member

balloob commented Oct 9, 2017

Woohoo Thanks 🐬

@balloob balloob changed the title add sort param to splitByGroups remove sorting from splitByGroups Oct 9, 2017
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.

3 participants