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

Add getAssignedNodes #322

Closed
rniwa opened this issue Sep 17, 2015 · 15 comments
Closed

Add getAssignedNodes #322

rniwa opened this issue Sep 17, 2015 · 15 comments

Comments

@rniwa
Copy link
Collaborator

rniwa commented Sep 17, 2015

We should be consistent with the terminology used elsewhere for slot attribute and assignedSlot and call this method getAssignedNodes instead.

Introducing new (or historical) terms like "distributed" would only confuse authors.

@hayatoito
Copy link
Contributor

I don't have a strong opinion on this either.

My concern is that the semantics of the assignedSlot and getAssignedNodes are not symmetry.
Using assigned for both might be confusing, however, I don't have any better name. :(

At least, I prefer getAssignedNodes to getComposedNodes, which was mentioned at #128.

@rniwa
Copy link
Collaborator Author

rniwa commented Sep 17, 2015

Right. The semantics will be same once we stop unwrapping slot (see #308) however.

@hayatoito
Copy link
Contributor

#308 is not related since it only affects the composition algorithm, I think.

Anyway, I'll rename it to getAssignedNodes unless there is a better proposal.
If someone disagree the renaming, please let us know it.

@rniwa
Copy link
Collaborator Author

rniwa commented Sep 18, 2015

Well, it is related in that if #308 gets resolved in the favor of keeping slot elements wrapped, then the get distributed nodes algorithm should also be changed to not flatten the distributed nodes in inner slot elements for consistency since it's quite trivial for authors to unwrap slots manually themselves.

@hayatoito
Copy link
Contributor

It's not just renaming. That's yet another proposal, changing the semantics of the getDistributedNodes.

I think we should rename this issue, "Replace getDistributedNodes with getAssignedNodes" or "Add getAssignedNodes() API".

Anyway, I understand your intention. That' exactly the same API which I thought to add to v1, but I haven't.

@hayatoito
Copy link
Contributor

If getAssignedNodes has the new semantics, I don't have any concern about its name.

@hayatoito hayatoito changed the title Rename getDistributedNodes() to getAssignedNodes() Replace getDistributedNodes with getAssignedNodes Sep 18, 2015
@hayatoito
Copy link
Contributor

I've renamed the issue. BTW, we can add getAssignedNodes without #308. #308 is not precondition of adding getAssingedNodes, I think.

@JanMiksovsky
Copy link

Naive question: instead of a function called getAssignedNodes, could this be a getter assignedNodes? To me, it feels similar to childNodes or children.

@rniwa
Copy link
Collaborator Author

rniwa commented Sep 18, 2015

The problem is that WebIDL doesn't allow sequence<T> to be used on an attribute so we'd have to use NodeList or HTMLCollection to add assignedNodes as a property as things stand.

@domenic
Copy link
Collaborator

domenic commented Sep 18, 2015

You could use FrozenArray<T>.

I think the more relevant question is whether this is an O(n) operation (use a method) or O(1) (use a property).

@rniwa
Copy link
Collaborator Author

rniwa commented Sep 18, 2015

How does FrozenArray work given the list of assigned nodes change over time? An array is either frozen or not frozen. We shouldn't be modifying a frozen array magically under the hood. If we're creating a new array on that attribute, then it's always O(n) regardless of whether it's a method or an attribute since we have to create a new array on each access.

@domenic
Copy link
Collaborator

domenic commented Sep 18, 2015

You would return a different frozen array whenever it changes, but the same one while it remains the same.

@annevk
Copy link
Collaborator

annevk commented Sep 21, 2015

I haven't really studied the details yet, but if the UA keeps an internal list of assigned nodes a property makes more sense. If it's something that is computed and we just return a snapshot a method makes more sense.

@hayatoito
Copy link
Contributor

I've added getAssignedNodes() as a method which returns sequence<Node>.
It should return a snapshot.

@hayatoito
Copy link
Contributor

I've not removed getDistributedNodes() yet.

@hayatoito hayatoito changed the title Replace getDistributedNodes with getAssignedNodes Add getAssignedNodes Oct 5, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants