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 Shadow DOM v1 APIs #151

Merged
merged 2 commits into from
Nov 14, 2016
Merged

Add Shadow DOM v1 APIs #151

merged 2 commits into from
Nov 14, 2016

Conversation

rictic
Copy link
Contributor

@rictic rictic commented Oct 4, 2016

@msftclas
Copy link

msftclas commented Oct 4, 2016

Hi @rictic, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by Microsoft and real humans are currently evaluating your PR.

TTYL, MSBOT;

@@ -3465,6 +3466,9 @@ interface Element extends Node, GlobalEventHandlers, ElementTraversal, NodeSelec
readonly scrollWidth: number;
readonly tagName: string;
innerHTML: string;
readonly assignedSlot: HTMLSlotElement | null;
Copy link
Contributor

Choose a reason for hiding this comment

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

The property is optional on the spec, we should make it optional here so that undefined can be accepted as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

And the same for other places too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I initially wrote it like that, but I when I was doing some double-checking it looked like WebIDL's ? suffix is different from typescript's ?. Take a look at this and let me know what you think:

So for places where the DOM accepts an IDL type T? from ES code then T | null | undefined looks like the right TS type, but for readonly properties it seems like T | null is more correct.

Apologies if this debate has already been litigated and the decision to treat nullable values as also allowing undefined has been made across the board.

I have made the non-required members of dictionaries, like EventInit#scoped optional.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the links. Yeah I agree it makes more sense, since the readonly types are from DOM itself.

Copy link
Contributor

Choose a reason for hiding this comment

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

I do not see why a readonly propoerty can not be undefined. this is useful for users who want to feature dection. this should be:

readonly assignedSlot?: HTMLSlotElement | null;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As I read the spec, in a browser that implements this API, assignedSlot will never be undefined.

It's true that in an environment without the API it could be undefined, but this is true of most browser APIs.

@rictic
Copy link
Contributor Author

rictic commented Nov 12, 2016

I noticed a copy-paste mistake in my earlier patch. I accidentally spelled HTMLSlotElement as ShadowRootInit. Rebased and pushed up a fixup commit, which can be squashed when this is merged.

@zhengbli zhengbli merged commit 0872515 into microsoft:master Nov 14, 2016
@zhengbli
Copy link
Contributor

Thank you @rictic for contributing!

@rictic rictic deleted the shadowDom branch November 14, 2016 20:10
@mhegazy
Copy link
Contributor

mhegazy commented Nov 14, 2016

the new properties should be marked optional.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants