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

Enable sumKeys option to sum more than one key when data are grouped #144

Merged
merged 16 commits into from
Jan 24, 2023

Conversation

stockiNail
Copy link
Collaborator

@stockiNail stockiNail commented Oct 13, 2022

Fix #47

New sumKeys option could be an array of keys of the objects used in tree and consumed grouping the data.
The result of sums of these additional keys will be stored in data object in vs key as object, where the keys are the same that sumKeys option has provided.

{
  "x": 0,
  "y": 32,
  "w": 714,
  "h": 682,
  "a": 486948,
  "v": 50,
  "vs": {
    "pop": 40
  },
  "s": 50,
  "g": "A",
  "l": 0
}

TODO

  • test cases
  • types
  • documentation

@stockiNail stockiNail added the enhancement New feature or request label Oct 13, 2022
@stockiNail stockiNail added this to the 2.2.0 milestone Oct 13, 2022
Conflicts:
	src/controller.js
	src/squarify.js
	test/specs/squarify.spec.js
@kurkle
Copy link
Owner

kurkle commented Nov 12, 2022

How about just allowing an array as key?
And internally, just make it always be an array, so the calculation is cleaner.
Just a thought, does not have to be that way.

@stockiNail
Copy link
Collaborator Author

I thought to use ah array but in this case we must assume the first element of the array is the key to use for grouping and drawing.
Personally I dont like this convention based on the position of an element in an array. I will have a look anyway next week if is not urgent

@kurkle
Copy link
Owner

kurkle commented Nov 13, 2022

The "position 1" convention can be kept internal and should only be used if the code is cleaner that way. Maybe sumKeys would be better name for it? Or additionalSumKeys.

@kurkle kurkle modified the milestones: 2.2.0, 2.3.0 Nov 22, 2022
Conflicts:
	test/fixtures/events/hoverCaptions.js
@stockiNail
Copy link
Collaborator Author

Maybe sumKeys would be better name for it? Or additionalSumKeys.

Done, in sumKeys.

@stockiNail
Copy link
Collaborator Author

stockiNail commented Jan 23, 2023

The "position 1" convention can be kept internal and should only be used if the code is cleaner that way.

Let me review better the code in order to merge key and sumKeys in an array (and then arguments for all other functions which are receiving that).

EDIT: apologize but I need more time to do something "good" enough" ;)

@stockiNail
Copy link
Collaborator Author

@kurkle when you have time, can you have a look if sounds better now? PR is still in draft.

Copy link
Owner

@kurkle kurkle left a comment

Choose a reason for hiding this comment

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

Yeah, this way looks good to me!

src/controller.js Outdated Show resolved Hide resolved
src/utils.js Outdated Show resolved Hide resolved
stockiNail and others added 2 commits January 23, 2023 21:19
Co-authored-by: Jukka Kurkela <jukka.kurkela@gmail.com>
@stockiNail stockiNail marked this pull request as ready for review January 23, 2023 20:32
@stockiNail stockiNail changed the title Enable additional keys option to sum more than one key when data are grouped Enable sumKeys option to sum more than one key when data are grouped Jan 23, 2023
docs/usage.md Outdated Show resolved Hide resolved
test/fixtures/grouped/treeAdditionalKeys.js Outdated Show resolved Hide resolved
@kurkle kurkle merged commit c122df1 into kurkle:main Jan 24, 2023
@stockiNail stockiNail deleted the newSumKeys branch January 25, 2023 07:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FR: Add option to use different property as rect area
2 participants