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

Fill in template contents for explainer repositories. #1

Merged
merged 7 commits into from
Nov 28, 2023

Conversation

jyasskin
Copy link
Member

@jyasskin jyasskin commented Nov 23, 2023

@tabatkins, let me know what changes you suggest for the initial Bikeshed file.

@jyasskin jyasskin requested a review from cwilso November 23, 2023 00:55
Copy link

@cwilso cwilso left a comment

Choose a reason for hiding this comment

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

Two requested changes:

  1. The instructions say "look for TODO" but really, each section needs to be filled out - either a bunch of sections need TODO in them, or just say "fill all this out".
  2. Maybe I'm misunderstanding the Key Scenarios sections (or it needs retitling to "API interactions" or something), but it seems like it needs to come further up, where use cases and scenarios are listed.


## Proponents

- [Proponent team 1]
Copy link

Choose a reason for hiding this comment

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

Should these have "todo" on them, or should the instructions say to look for brackets?

Copy link
Member Author

Choose a reason for hiding this comment

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

I hope moving the "TODO: Fill in the whole explainer template" bit above this is sufficient.

README.md Outdated
- [etc.]

## Participate
- [Issue tracker]
Copy link

Choose a reason for hiding this comment

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

As above: todo here?

README.md Outdated

## Introduction

TODO: Fill this in using https://tag.w3.org/explainers/ as a reference.
Copy link

Choose a reason for hiding this comment

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

It's not clear if this TODO is for the entire following sections or not?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've moved it up to the top of the explainer and reworded a bit so it's clear it covers the whole thing.

README.md Outdated

[If spec work is in progress, link to the PR or draft of the spec.]

## [Potential Solution 2]
Copy link

Choose a reason for hiding this comment

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

This is possible, but relatively unlikely (that a second+ solution would be documented, right? Perhaps we should remove this, but above say "there may be more than one potential solution section".

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good.

README.md Outdated
<!-- In your initial explainer, you shouldn't be attached or appear attached to any of the potential
solutions you describe below this. -->

## [Potential Solution 1]
Copy link

Choose a reason for hiding this comment

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

The potential solution(s) should be below the "tricky design choice" section below, but DEFINITELY after the Key Scenarios.

README.md Outdated
and a brief overview of how the solution works.
This should be no more than 1-2 paragraphs.]

## Goals [or Motivating Use Cases, or Scenarios]
Copy link

Choose a reason for hiding this comment

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

Not clear how this interacts with the Key Scenarios below? Definitely need to make sure htis gets filled out, in depth, prior to the potential solution.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah. I took this directly from the TAG's template, but it's not clear. I think the idea was to list use cases here, and then have the Key Scenarios explain how the potential solutions solve the use cases. I'll try to make that clearer ... and then also send the TAG a patch.

index.bs Outdated

For now, see the [explainer]([REPOSITORYURL]).

See [https://speced.github.io/bikeshed/](https://speced.github.io/bikeshed/) to get started on your specificaton.

Choose a reason for hiding this comment

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

Drop in links to the other "how to Bikeshed" and "how to spec" links? So people have several jumping off points, besides just the (sometimes intimidating) Bikeshed docs.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

index.bs Outdated
Repository: explainers-by-googlers/todo-your-repo-name
URL: https://explainers-by-googlers.github.io/todo-your-repo-name
Editor: TODO: Your Name, Google https://google.com, TODO@google.com
!Tests: <a href=https://github.com/w3c/web-platform-tests/tree/master/todo-api-label>web-platform-tests todo-api-label/</a> (<a href=https://github.com/w3c/web-platform-tests/labels/todo-api-label>ongoing work</a>)

Choose a reason for hiding this comment

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

Is there a particular reason you're suggesting an explicitly-constructed WPT line, rather than using the Bikeshed's WPT features directly? This method won't, for example, fill in any test results.

Suggest instead a WPT Path Prefix: TODO-API-LABEL and WPT Display: closed, along with a <wpt rest></wpt> at the end of the body along with a short note explaining how to properly sprinkle <wpt> thruout the document.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is what npx wicg produced: https://github.com/WICG/starter-kit/blob/main/templates/index.bs. I've taken your suggestion, and I'll update that once this template is merged.

Copy link
Member Author

@jyasskin jyasskin left a comment

Choose a reason for hiding this comment

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

Thanks for the reviews. How's this?

index.bs Outdated
Repository: explainers-by-googlers/todo-your-repo-name
URL: https://explainers-by-googlers.github.io/todo-your-repo-name
Editor: TODO: Your Name, Google https://google.com, TODO@google.com
!Tests: <a href=https://github.com/w3c/web-platform-tests/tree/master/todo-api-label>web-platform-tests todo-api-label/</a> (<a href=https://github.com/w3c/web-platform-tests/labels/todo-api-label>ongoing work</a>)
Copy link
Member Author

Choose a reason for hiding this comment

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

This is what npx wicg produced: https://github.com/WICG/starter-kit/blob/main/templates/index.bs. I've taken your suggestion, and I'll update that once this template is merged.

index.bs Outdated

For now, see the [explainer]([REPOSITORYURL]).

See [https://speced.github.io/bikeshed/](https://speced.github.io/bikeshed/) to get started on your specificaton.
Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

README.md Outdated

[If spec work is in progress, link to the PR or draft of the spec.]

## [Potential Solution 2]
Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good.

README.md Outdated

## Introduction

TODO: Fill this in using https://tag.w3.org/explainers/ as a reference.
Copy link
Member Author

Choose a reason for hiding this comment

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

I've moved it up to the top of the explainer and reworded a bit so it's clear it covers the whole thing.


## Proponents

- [Proponent team 1]
Copy link
Member Author

Choose a reason for hiding this comment

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

I hope moving the "TODO: Fill in the whole explainer template" bit above this is sufficient.

README.md Outdated
and a brief overview of how the solution works.
This should be no more than 1-2 paragraphs.]

## Goals [or Motivating Use Cases, or Scenarios]
Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah. I took this directly from the TAG's template, but it's not clear. I think the idea was to list use cases here, and then have the Key Scenarios explain how the potential solutions solve the use cases. I'll try to make that clearer ... and then also send the TAG a patch.

index.bs Outdated
and [https://speced.github.io/bikeshed/](https://speced.github.io/bikeshed/) to get started on your
specificaton.

<wpt rest></wpt>
Copy link
Member Author

Choose a reason for hiding this comment

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

Hmmm, @tabatkins this spelling produces an "Attribute “rest” not allowed on element “details” at this point." error in spec-prod's markup validation. Using <wpt-rest> as suggested in https://speced.github.io/bikeshed/#elementdef-wpt-rest fails Bikeshed with

FATAL ERROR: Couldn't find any tests with the path prefix 'TODO-API-LABEL'.

I could just remove this or comment this out, since the issue tells people what to do once they set their WPT Path Prefix. What do you think?

Choose a reason for hiding this comment

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

Sorry, yes, <wpt-rest> is what I meant.

Re: the fatal error; go ahead and just ignore my suggestion for a wpt-rest, actually. It works just fine as-is; it (currently) won't complain at all about a nonsense path prefix as long as you don't actually use any wpt-related elements. But when that TODO is replaced with the actual path prefix, it'll start complaining about tests not being specified, which is good. (Tho I really need to work on the error message...)

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed. :)

When the spec author fills in the WPT Path Prefix, it'll give them
errors that say to add <wpt> elements, which is as good as an inline
issue.
@jyasskin jyasskin merged commit 2e383d8 into main Nov 28, 2023
2 checks passed
@jyasskin jyasskin deleted the initial-template branch November 28, 2023 21:29
github-actions bot added a commit that referenced this pull request Nov 28, 2023
SHA: 2e383d8
Reason: push, by jyasskin

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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