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

New contributor docs #1330

Merged
merged 14 commits into from
Aug 13, 2024
Merged

New contributor docs #1330

merged 14 commits into from
Aug 13, 2024

Conversation

captainbrosset
Copy link
Contributor

@captainbrosset captainbrosset commented Jul 5, 2024

This is an attempt to fix #1057.
Draft for now, I've not yet written everything I wanted to write. Arguably the most important section is still just a TODO.

Pinging @ddbeck and @foolip though, since they might have early feedback about the TODO at the bottom of the file.

To more reasily review, use this rendered markdown: https://github.com/web-platform-dx/web-features/blob/docs/docs/CONTRIBUTING.md

docs/CONTRIBUTING.md Outdated Show resolved Hide resolved
docs/CONTRIBUTING.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@ddbeck ddbeck left a comment

Choose a reason for hiding this comment

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

I think this is a really strong start—I've made a note about the one omission that hasn't already been mentioned. I'll have some nits to pick when it gets to that stage, but so far so good.

docs/CONTRIBUTING.md Outdated Show resolved Hide resolved
docs/CONTRIBUTING.md Outdated Show resolved Hide resolved
docs/CONTRIBUTING.md Outdated Show resolved Hide resolved
@captainbrosset captainbrosset requested review from ddbeck and foolip July 11, 2024 08:58

Each section corresponds to a group of BCD keys that have the same baseline and support status.

_TODO: continue documenting this section (consider splitting BCD keys into different features (later addition), or using just silencing some BCD keys by using `compat_from`)._
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the last part that I haven't yet documented. And it also happens to be the most tricky. I could use help figuring out how far to go. I feel like a lot of the times, this will be resolved by discussions in PRs, so I'm not sure how far I should go in trying to document each and every case.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, I feel like this might be simplified a bit for now, until we have a better handle on common cases. I might simplify this whole section as having three key parts: too old, too new, and too wrong.

  1. If the feature's overall status is too old (e.g., it's a new feature being reported as long-established), then you might have missing compat keys. Check that you've covering the complete feature by looking for missing interfaces, CSS property values, and so on.
  2. If the feature's overall status is too young (e.g., a long-established feature is reported as newly available), then you might have later additions holding the feature back unfairly. Consider finding a small number of entry points into the feature that are representative of the whole (e.g., the api.fetch key can stand in for support for Fetch in general), then using those entry points in the compute_from field.
  3. If the feature's overall status and individual keys are yielding incorrect results, the underlying data may have caveats or errors. Check mdn/browser-compat-data. Important caveats include features behind prefixes or flags, or partial implementations. Errors include missing support (e.g., a feature shown as supported in a desktop browser but not mobile) or wrong version numbers.

Copy link
Collaborator

Choose a reason for hiding this comment

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

My approach is to look for the feature's entrypoint in the dist file. If that entrypoint doesn't have the "⬇️ Same status as overall feature ⬇️" comment I use compute_from to make it so. Whether the feature was too old or too new is something you find out this way, at least I don't do a different thing depending on whether I think it's too old or too new initially.

@captainbrosset
Copy link
Contributor Author

For info, the content is easier to review by looking at the rendered markdown here: https://github.com/web-platform-dx/web-features/blob/docs/docs/CONTRIBUTING.md

docs/CONTRIBUTING.md Outdated Show resolved Hide resolved

Each section corresponds to a group of BCD keys that have the same baseline and support status.

_TODO: continue documenting this section (consider splitting BCD keys into different features (later addition), or using just silencing some BCD keys by using `compat_from`)._
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, I feel like this might be simplified a bit for now, until we have a better handle on common cases. I might simplify this whole section as having three key parts: too old, too new, and too wrong.

  1. If the feature's overall status is too old (e.g., it's a new feature being reported as long-established), then you might have missing compat keys. Check that you've covering the complete feature by looking for missing interfaces, CSS property values, and so on.
  2. If the feature's overall status is too young (e.g., a long-established feature is reported as newly available), then you might have later additions holding the feature back unfairly. Consider finding a small number of entry points into the feature that are representative of the whole (e.g., the api.fetch key can stand in for support for Fetch in general), then using those entry points in the compute_from field.
  3. If the feature's overall status and individual keys are yielding incorrect results, the underlying data may have caveats or errors. Check mdn/browser-compat-data. Important caveats include features behind prefixes or flags, or partial implementations. Errors include missing support (e.g., a feature shown as supported in a desktop browser but not mobile) or wrong version numbers.

Copy link
Member

@tidoust tidoust left a comment

Choose a reason for hiding this comment

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

Excellent start! I added a few comments. One general comment is that features have two heads:

  1. There's one head that is more about "naming": choosing an ID, a name, setting the description, the URLs to the spec(s). Although technical knowledge is no doubt needed to understand how to scope a feature, that seems to be doable by a large number of people.
  2. There's one head that is more about the support data. That part requires more intimate knowledge about BCD, browser versions and history. That's more for "experts".

I'm wondering whether we might want to separate the two in the submission process: Want to propose a feature? First step, give us the "naming" part and don't worry too much about the support data for now if you're not familiar enough. Second step, which could be done by others, focus on the support data.

Now, I do realize that the two heads are intertwined: the way you name and describe a feature depends on the exact set of compat keys that get associated with it, so not sure there's much that can be done in practice.

docs/CONTRIBUTING.md Outdated Show resolved Hide resolved
docs/CONTRIBUTING.md Outdated Show resolved Hide resolved
| `group` | An optional group, or list of groups that this feature belongs to | String, or array of strings | No |
| `snapshot` | An optional snapshot, or list of snapshots that this feature belongs to | String, or array of strings | No |
| `caniuse` | The feature's ID on the [Can I Use](https://caniuse.com/) website. | String | No |
| `compat_features` | The list of browser-compat-data entries that make up this feature. In many cases, the corresponding browser-compat-data entries already map to the feature ID, and this is therefore not needed. | Array of strings | No |
Copy link
Member

Choose a reason for hiding this comment

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

There's a bit of a contradiction in the doc: the table suggests "easy-peasy, just don't worry about that", while the steps insist on providing "the necessary information for this feature's status to be calculated" (and says that's tough).

I'm also not clear about our intended workflow in practice: The compat_features essentially needs to be provided for each new feature because people proposing new features are unlikely going to be BCD experts (except for a handful of people who already pay close attention to web-features). When does the data gets pushed to BCD?
? If so, when and how do we push the data to BCD? (I vaguely recall that there is a script that does that automatically)? If not, how do we want to proceed?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Very good questions, @tidoust!

compat_features is the easiest way to author a new feature, but then we want tags in BCD for the long term, so that web-features doesn't have to react to renames and the like.

I have a script in #473 to migrate tags, but I've only run it 2 or 3 times, there's no schedule.

A situation which is likely to get contributors confused is when BCD has tags but you need to add or remove something. Then you need to list all of the keys that should be included in compat_features, completely ignoring BCD. But how do you get back in sync with BCD? The current answer is by hand, with some pain.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This goes back to your general comment @tidoust, stating that it might make sense to almost have 2 contribution guides (or 2 sections). One where you focus on creating the feature at all, with a good ID, name, description. And one, more involved, where you spend time identifying the right BCD keys and which ones the status should be computed from.
I'm not against that. I like beginner-friendly guides that help newcomers get from nothing to a PR.
Since this is a larger piece of work, how about we do this in a separate PR?

docs/CONTRIBUTING.md Outdated Show resolved Hide resolved
spec: https://urlofthespec.com
```

For guidance, see the [Names](./guidelines.md#names) and [Descriptions](./guidelines.md#descriptions) sections of the feature guidelines.
Copy link
Member

Choose a reason for hiding this comment

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

Another place where a highllight could be used.
It would also be cool if we had guidance on spec: what to link to, when to have mulitple URLs.


For guidance, see the [Names](./guidelines.md#names) and [Descriptions](./guidelines.md#descriptions) sections of the feature guidelines.

1. Optionally add a `group` field to the feature.
Copy link
Member

Choose a reason for hiding this comment

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

I don't have text to suggest immediately, but it would be great to have some sort of purpose here. That is, without knowing what groups are for (other than "to categorize features"), it's hard to understand whether they're worth spending time on.

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 don't think we have a purpose for groups yet. We need to work with consumers to learn what they could do with groups. If they're not going to use groups, then my personal opinion would be to turn them into a learning resource: something you can use to logically learn what's available in the web platform. But that's just one way to group features.

Perhaps we can remove this section for now, and only add it once we know what we want to do with groups?

docs/CONTRIBUTING.md Outdated Show resolved Hide resolved
docs/CONTRIBUTING.md Outdated Show resolved Hide resolved
docs/CONTRIBUTING.md Outdated Show resolved Hide resolved
docs/CONTRIBUTING.md Show resolved Hide resolved
docs/CONTRIBUTING.md Outdated Show resolved Hide resolved
docs/CONTRIBUTING.md Outdated Show resolved Hide resolved
docs/CONTRIBUTING.md Outdated Show resolved Hide resolved
docs/CONTRIBUTING.md Outdated Show resolved Hide resolved
docs/CONTRIBUTING.md Outdated Show resolved Hide resolved
docs/CONTRIBUTING.md Outdated Show resolved Hide resolved
docs/CONTRIBUTING.md Outdated Show resolved Hide resolved
docs/CONTRIBUTING.md Outdated Show resolved Hide resolved
docs/CONTRIBUTING.md Outdated Show resolved Hide resolved
@foolip
Copy link
Collaborator

foolip commented Jul 15, 2024

@captainbrosset this is looking really good to me! Is there something left you want to do before marking this ready for review (not draft)?

@captainbrosset
Copy link
Contributor Author

captainbrosset commented Jul 16, 2024

@captainbrosset this is looking really good to me! Is there something left you want to do before marking this ready for review (not draft)?

Yes, I want to resolve the TODO in Fix data discrepancies in your feature's dist file.
There's more to do, but I think it's probably enough of a start, and we can continue to improve it over time.

@captainbrosset captainbrosset marked this pull request as ready for review July 16, 2024 11:35
@captainbrosset
Copy link
Contributor Author

@ddbeck @tidoust @foolip thank you for the reviews so far. I'm now done with the content I wanted to write. Again, there's more to do here, such as:

  • Guidance about specs.
  • Guidance about groups.
  • Splitting the docs into 2 parts: one, simpler, part to just get a feature started. And another, more involved, part to resolve data discrepancies.
  • Better documentation for the compute_from field.
  • Adding missing docs for completely overriding the status.

I'd prefer to track this in a new issue so we can add the missing docs over time in subsequent PRs.

docs/CONTRIBUTING.md Outdated Show resolved Hide resolved
docs/CONTRIBUTING.md Outdated Show resolved Hide resolved
docs/CONTRIBUTING.md Outdated Show resolved Hide resolved
docs/CONTRIBUTING.md Outdated Show resolved Hide resolved
@ddbeck ddbeck added the documentation Improvements or additions to documentation label Jul 26, 2024
@captainbrosset
Copy link
Contributor Author

I think I've addressed most comments on this first version, and I'd like to merge this sooner rather than later to avoid blocking other docs changes/merging conflicts for a long period of time.
I've seen enough positive reactions here to make me confident that this is enough of a useful start to merge, and then continue iterating over subsequent PRs.

@ddbeck do you agree?

Copy link
Collaborator

@ddbeck ddbeck left a comment

Choose a reason for hiding this comment

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

Yes, I'm happy for this to merge. Thank you!

@ddbeck ddbeck merged commit e29cc8c into main Aug 13, 2024
3 checks passed
@ddbeck ddbeck deleted the docs branch August 13, 2024 13:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Contributing guidelines
5 participants