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

Unclear usage of word 'group' #16

Open
siccovansas opened this issue Dec 27, 2021 · 1 comment
Open

Unclear usage of word 'group' #16

siccovansas opened this issue Dec 27, 2021 · 1 comment
Assignees
Labels
documentation Improvements or additions to documentation enhancement New feature or request

Comments

@siccovansas
Copy link

In in the JavaScript API section of the readme.md the word 'group' is used at several places. I thought this was confusing as I thought it would be about data-match-height-group, but that doesn't seem to be the case. It might be better to use another term than 'group' there?

PS: great package, thx!

@tannerhodges
Copy link
Owner

🤦‍♂️ Yes, this is definitely confusing and needs improvement.

What does "group" mean?

In hindsight, I realize I mixed two different ideas together:

  • IDs: When you assign an ID with data-match-height, that creates a "group" of elements. ← I should have stopped here and only used the term "group" to refer to a group of elements—not a group of groups of elements, which is what I meant for data-match-height-group to do.
  • Prefixes: The actual implementation for data-match-height-group ended up being a prefix. Instead of truly creating groups of grouped elements, I tried to keep the internal logic simple by only prefixing the IDs with the extra "group" value. ← This is where things get confusing. The internal logic doesn't match the model's terms: when you refer to a group of elements, the code expects an ID, not a parent group name, not a prefix… So none of the references to "group" in the JS API actually have anything to do with data-match-height-group (the prefix), they all exclusively use the ID (the final concatenated ID, including the parent group—the prefix—in the name if one exists).

In other words, anytime you see the term "group" in the JS API docs, it really means the computed group ID

  • Computed Group ID = Prefix + Group ID.
  • getGroupID() = data-match-height-group + - + data-match-height.
<div data-match-height-group="example">
  <div data-match-height="box"></div>
</div>

☝️ In this example, the computed group ID = example-box.

Gross… Writing that all out makes me instantly regret the decision. Again, this definitely needs improvement.

How should we fix this?

I feel this will probably require a breaking change (more than just a docs update). Two quick thoughts come to mind for how we could resolve the confusion:

  • Update JS APIs to acknowledge prefixes as groups: We could try and resolve the discrepancy by restructuring how this.groups and getGroups() work, making them first check for "parent groups" (i.e., data-match-height-group) and update all "element groups" inside of them.
  • Rename data-match-height-group to data-match-height-prefix: Rather than do a large restructuring, we could refactor some of the attribute & variable names to clearly express how the plugin currently works—distinguish between IDs (i.e., groups) and prefixes.

Personally, I lean towards renaming (less work). But there are likely other, more elegant solutions I haven't thought of yet.

Thoughts?

@tannerhodges tannerhodges self-assigned this Jan 3, 2022
@tannerhodges tannerhodges added documentation Improvements or additions to documentation enhancement New feature or request labels Jan 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants