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

Fix #280 and 281: add emitPropertyChange(), remove ConsumedThing inheritance #283

Merged
merged 2 commits into from Nov 16, 2020

Conversation

zolkis
Copy link
Contributor

@zolkis zolkis commented Nov 11, 2020

add explicit emitPropertyChange() and remove ConsumedThing inheritance from ExposedThing

Signed-off-by: Zoltan Kis zoltan.kis@intel.com


Preview | Diff

Copy link
Member

@relu91 relu91 left a comment

Choose a reason for hiding this comment

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

Approve. Minor comments:

  • Could we link the first editor's note also in the other algorithms? it might be handy when the document is read not linearly.
  • Could we use camel case for ExposedThing internal slot extd. It might be confused with a shortened version of extend whereas it should be ExposedThingDescription. So it might be more intuitive to call it exTD or exTd.

@zolkis
Copy link
Contributor Author

zolkis commented Nov 12, 2020

Could we use camel case for ExposedThing internal slot extd. It might be confused with a shortened version of extend whereas it should be ExposedThingDescription. So it might be more intuitive to call it exTD or exTd.

Makes sense, done.

Could we link the first editor's note also in the other algorithms? it might be handy when the document is read not linearly.

The algorithms say "selected by the implementation", which should be clear enough even without the ednote. Repeating the ednote everywhere would be too much clutter, given that it just repeats the last sentence. Perhaps we could remove even the first one?

@danielpeintner
Copy link
Contributor

Could we use camel case for ExposedThing internal slot extd. It might be confused with a shortened version of extend whereas it should be ExposedThingDescription. So it might be more intuitive to call it exTD or exTd.

Makes sense, done.

Nit: I am always in favor of spelling out "what" it is since exTD sounds like example TD to me. Why not name it along the lines what it is: exposedTD.
Since it is internal only I don't mind much.

Copy link
Contributor

@danielpeintner danielpeintner left a comment

Choose a reason for hiding this comment

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

I am generally fine with the PR.

I added some comments that seem beneficial to me.

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Show resolved Hide resolved
…sumedThing inheritance from ExposedThing

Signed-off-by: Zoltan Kis <zoltan.kis@intel.com>
Signed-off-by: Zoltan Kis <zoltan.kis@intel.com>
@zolkis zolkis merged commit 0ba3349 into w3c:master Nov 16, 2020
@zolkis zolkis deleted the remove-inheritance branch November 16, 2020 13:50
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.

None yet

3 participants