-
Notifications
You must be signed in to change notification settings - Fork 21
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
Update status of this document #61
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @reillyeon. Addressed your suggestion in an update, also updated the commit message.
Could you open a new issue linking to the proposal from 2014 you mentioned?
I'd like to link to that issue from this status text as to raise awareness of options that have been considered and to invite feedback and comments from the web community on that proposal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like spec-prod validator does not like [REPOSITORYURL] text macro inside href, CI error:
Local build was fine. May need to revert back to the plain old URL unless some Bikeshed expert tells me otherwise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't define repository in macro, so add repository line (with
text macro: REPOSITORY w3c/geolocation-sensor
etc.)(fixed)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(wrong again...) text macro will be created following
Repository:
line, and should be created automatically if bikeshed runs via ghaction... hrm.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR updated. The rendered version will have a very visible issue block on top of the API section pointing to that issue. The status text links to the API section too setting expectations with regard to changes expected.
Also fixed the text macro, thanks @himorin for filing an issue for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is fine but please remove the "Fix #60" comment in the description as this does not resolve the issue raised there. To be clear, I think there are two paths forward for resolving that issue. Either,
I recommend (1). I don't foresee (2) being possible because there has been no interest for many years in implementing this API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 for (1)!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@reillyeon thanks for the suggestions. I removed the left-over "Fix" from the comment. PTAL and merge at will.
Some considerations:
A process-related clarification for option (1):
Given this spec is a Working Draft, and given (1) proposes a narrower scope, the WG only needs to decide to publish an updated Working Draft with foreground functionality removed without any heavy-handed mechanisms beyond that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, we can just modify the Working Draft.