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

Editorial fixes #100

Merged
merged 9 commits into from
Jul 26, 2016
Merged

Editorial fixes #100

merged 9 commits into from
Jul 26, 2016

Conversation

marcoscaceres
Copy link
Member

No description provided.

might only expose the status to web pages when they try to use the API,
like the [[geolocation-API]] which fails if the permission was not
expose the status to web pages when they try to use the API,
like the [[Geolocation-API]] which fails if the permission was not
Copy link
Member

Choose a reason for hiding this comment

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

Bikeshed wants you to change the [[geolocation-API]] in #geolocation too.

Copy link
Member Author

Choose a reason for hiding this comment

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

ah, boo. I thought it would support case insensitive look-up for bibref.

Copy link
Member

Choose a reason for hiding this comment

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

Both of them match; it's Bikeshed saying that we should use a consistent spelling of a [[reference]] within a given spec.

@jyasskin
Copy link
Member

There's a bunch of bikeshed errors here. Could you run bikeshed over the file and fix them?

@marcoscaceres
Copy link
Member Author

Will do! Sorry, still new to bs.

@marcoscaceres
Copy link
Member Author

trying to address BS fatal errors... getting strange ones like:
FATAL ERROR: Unknown metadata key "Repository". Prefix custom keys with "!".

@@ -128,7 +123,7 @@ spec: webidl
or other sources this specification hasn't anticipated.
</dd>

<dt><dfn export>Powerful feature</dfn></dt>
<dt><dfn export lt="powerful feature|feature">Powerful feature</dfn></dt>
Copy link
Member

@jyasskin jyasskin Jul 13, 2016

Choose a reason for hiding this comment

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

Missed the possibility of local-lt here too: Probably we don't want this spec to define "feature" for the whole world? And then you can just remove the lt= since that's the content of the tag. Sorry for the bad suggestion earlier.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, <dfn export local-lt=feature>Powerful feature</dfn> is the ideal markup here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, fixed both.

@jyasskin
Copy link
Member

trying to address BS fatal errors... getting strange ones like:
FATAL ERROR: Unknown metadata key "Repository". Prefix custom keys with "!".

https://api.csswg.org/bikeshed/?url=https://raw.githubusercontent.com/w3c/permissions/editorial_fixes/index.bs&output=both doesn't have any fatal errors, so that sounds like something in your bikeshed installation is broken. Repository is a known key, too. @tabatkins, any idea what it would be?

@tabatkins
Copy link
Member

Yeah, that'd be an old Bikeshed version. Go to your bikeshed folder and run git pull --rebase

@marcoscaceres
Copy link
Member Author

@jyasskin, I explicitly marked "read-current-permission-state" as an algorithm. Is that ok?

@marcoscaceres
Copy link
Member Author

@jyasskin, see 18f6961

same value, unless the UA receives <a>new information about the user's
intent</a>.
</p>
<p>
Copy link
Member

Choose a reason for hiding this comment

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

This is a different algorithm, defining the result of

"permission-name"'s extra permission data

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, I don't understand this comment :(

In case I don't hear back from you in the next few hours; I'm about to shift focus to another project for 2 weeks, so could I kindly request that you fix this for me in the editorial_fixes branch? I've already fixed the <var>s (there are ~30 vars that could maybe be converted also, but I guess we should do that in a new PR).

Copy link
Member

Choose a reason for hiding this comment

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

Oh yeah, totally. I'd forgotten that this branch was in the main repo rather than a personal fork. I'll take it over.

@jyasskin
Copy link
Member

Ok, everything except the details of 18f6961 looks good.

@marcoscaceres
Copy link
Member Author

rebased.

@marcoscaceres
Copy link
Member Author

(don't forget to merge me or there will be much bit rot 👍 )

@jyasskin jyasskin merged commit c9838be into master Jul 26, 2016
@jyasskin jyasskin deleted the editorial_fixes branch July 26, 2016 23:41
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.

3 participants