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

Rename getAssignedNodes()? #451

Closed
annevk opened this issue Mar 22, 2016 · 19 comments
Closed

Rename getAssignedNodes()? #451

annevk opened this issue Mar 22, 2016 · 19 comments

Comments

@annevk
Copy link
Collaborator

annevk commented Mar 22, 2016

  1. findSlotables() seems nicer and clearer it's a little expensive.
  2. findSlotables({distributed:true}) also seems a little clearer. Neither Shadow DOM nor DOM uses flatten for this algorithm.
@rniwa
Copy link
Collaborator

rniwa commented Mar 22, 2016

findSlotables sounds like it'll find all slottable candidates under shadow host. Maybe getSlottedNodes?

@treshugart
Copy link

Does this mean that Element.assignedSlot should be renamed as well? getSlottedNodes() is nice, but strays from the convention of using "assign".

@hayatoito
Copy link
Contributor

I do not like using find as the name of this web-exposed API. It would impress me that UA actually starts to find. I would like to avoid this. Thus, I prefer getter-like name here. If possible, omit get.

How about Slot.slottedNodes() and {Element/Text}.assignedSlot()?

Regarding, {distributed:true}, I prefer flatten to distributed. But I do not think flatten is the best word.
We never use distributed nor flatten as web-exposed names so far. Any better idea is welcome.

@annevk
Copy link
Collaborator Author

annevk commented Mar 23, 2016

assignedNode -> slotNode? I would be okay with slottedNodes(). Even just slotted() if we wanted to be consistent with deepPath() (it's not deepPathNodes() after all).

No good ideas for flat/distributed.

@hayatoito
Copy link
Contributor

Okay. My preference, as of now, is:

  • Slot.getAssingedNodes() -> Slot.slotted({'flatten': true})
  • {Text/Element}.assignedSlot() -> {Text/Element}.assignedSlot() (Will not change)

I am assuming that {Text/Element}.slot() is a bad idea because Element has a slot attribute.
Is my understanding correct?

@JanMiksovsky
Copy link

I don't think that slotted is sufficiently clear. Unlike deepPath, "slotted" is an adjective. To someone who doesn't already know what the property does, it could sound like the property is describing the state of the Slot element itself. Compare with, say, an HTMLInputElement that has a checked property that indicates if the element is checked or not. Similarly, a slotted property on an element could sound like that element is in the state of having been put into a slot.

I think slottedNodes is better for this reason. I think assignedNodes (just dropping the "get) would be equally good.

@hayatoito
Copy link
Contributor

Thanks. Then,

  • Slot.getAssingedNodes() -> Slot.slottedNodes({'flatten': true})
  • {Text/Element}.assignedSlot() -> {Text/Element}.assignedSlot() (Will not change)

is okay to everyone?

@annevk
Copy link
Collaborator Author

annevk commented Mar 24, 2016

I think if we don't rename assignedSlot to slotNode or some such, I'm not sure we should rename the other one. Either we keep using "assigned" consistently (even though technically they're found) or we switch to something new. Maybe it's best to just preserve the status quo then.

@annevk
Copy link
Collaborator Author

annevk commented Mar 28, 2016

Given #288 I suspect assigned might become accurate after all since the evaluation won't be lazy in that case but a direct result of running the insert/remove algorithms. It would help if we could make a decision of sorts soonish there.

It's still a little unclear to me when we use get() and when we leave it out though. I would personally also be fine with something like assigned().

@hayatoito
Copy link
Contributor

We should make a decision.

Option A: Preserve the status quo

  • Slot.getAssingedNodes(...) -> (as is)
  • {Text/Element}.assignedSlot -> (as is)

Option B: Remove get for consistency

  • Slot.getAssingedNodes(...) -> Slot.assignedNodes(...)
  • {Text/Element}.assignedSlot -> (as is)

Option C: Use slotted

  • Slot.getAssingedNodes(...) -> Slot.slottedNodes(...)
  • {Text/Element}.assignedSlot -> (as is)

I prefer option B.

[Update: remove unnecessary parenthesis]

@annevk
Copy link
Collaborator Author

annevk commented Mar 31, 2016

It's assignedSlot btw, no parenthesis, just a getter.

@annevk
Copy link
Collaborator Author

annevk commented Mar 31, 2016

I agree with B, @rniwa?

@rniwa
Copy link
Collaborator

rniwa commented Mar 31, 2016

It's strange not to have "get" prefix IMO given other DOM APIs seem to have that. e.g. getElementsByTagName, getAttribute, etc... Having said that, I don't have a strong opinion here other than I don't like C.

@treshugart
Copy link

Is there a reason for keeping getAssignedSlots() a method? Why not simply a getter assignedNodes? I realise that you can specify { distributed: true }, but creating a separate property for that like distributedNodes would seem like it creates a similar consistency that childNodes has with children. It also seems more consistent with the rest of the Node / Element API.

@hayatoito
Copy link
Contributor

I think a method is preferred if it returns (non-live) sequence<T>.

childNodes (or children) returns a live collection, I guess.

BTW, I forgot to mention that Slot.getAssingedNodes(...) is a method and {Text/Element}.assignedSlot is not a method in
#451 (comment).

If we prefer get prefix for a method, I'm fine with Option A (Preserve the status quo), either.

Thus, unless there is a strong preference, I would like to preserve the status quo.

@annevk
Copy link
Collaborator Author

annevk commented Apr 1, 2016

Yeah, we cannot do a getter for non-live lists. We don't want x.prop !== x.prop.

@hayatoito
Copy link
Contributor

Unless there is a strong preference, I'll preserve the status quo (Option A).

@annevk
Copy link
Collaborator Author

annevk commented Apr 5, 2016

I'd prefer B since static lists don't have the "get" prefix thus far.

@hayatoito
Copy link
Contributor

Okay. Then, option B sounds the conclusion. Let me update the spec.

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

6 participants