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

ARIAMixin #2001

Merged
merged 16 commits into from
Feb 25, 2021
Merged

ARIAMixin #2001

merged 16 commits into from
Feb 25, 2021

Conversation

rachelandrew
Copy link
Collaborator

@rachelandrew rachelandrew commented Feb 2, 2021

@jpmedley I have done just one of these properties to check that you are happy with this approach. They are all essentially going to be the same so if we are happy with one it will just be a case of rattling through the others.

Notes:

In our spreadsheet the mixin is listed as AriaAttributes, in the updated spec (and mentioned in the HTML spec) this is now ARIAMixin with updated detail on getting and setting. I think in terms of Element it is essentially still a reflection.

This is taking the approach as discussed in #1940

Two questions:

  1. Do we then need to create BCD in the giant Element table?
  2. Should I add a section for ARIA on the page https://developer.mozilla.org/en-US/docs/Web/API/Element to link to all of these?

reviewer @jpmedley

@rachelandrew rachelandrew requested a review from a team as a code owner February 2, 2021 10:37
@rachelandrew rachelandrew requested review from sideshowbarker and removed request for a team February 2, 2021 10:37
@rachelandrew rachelandrew marked this pull request as draft February 2, 2021 10:37
@rachelandrew rachelandrew requested review from jpmedley and removed request for sideshowbarker February 2, 2021 10:37
@jpmedley jpmedley self-assigned this Feb 2, 2021
@nschonni
Copy link
Contributor

nschonni commented Feb 3, 2021

@rachelandrew
Copy link
Collaborator Author

@nschonni I don't think so, this is the interface.

@rachelandrew
Copy link
Collaborator Author

I think the outcome of the discussion here is the answer to my first question above: mdn/browser-compat-data#8929

I'll get on tomorrow with the rest of these.

@rachelandrew rachelandrew marked this pull request as ready for review February 5, 2021 12:37
@rachelandrew
Copy link
Collaborator Author

@jpmedley here you go, all 41 pages of it. They are all very similar however and based off the initial one you saw, so other than egregious typos hopefully reviewing is not the epic that writing them was!

@rachelandrew
Copy link
Collaborator Author

I have added the BCD line according to what the suggested new way of doing these is. If that's what we end up going with I can look at adding the compat data for these.

Copy link
Collaborator

@jpmedley jpmedley left a comment

Choose a reason for hiding this comment

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

Rachel,

Thank you for doing this. I know this submission was probably a tedious slog.

Many of the changes I'm asking for need to be done across the collection. Let me know if you need help.

Joe

files/en-us/web/api/element/ariaatomic/index.html Outdated Show resolved Hide resolved
files/en-us/web/api/element/ariaautocomplete/index.html Outdated Show resolved Hide resolved
files/en-us/web/api/element/ariaautocomplete/index.html Outdated Show resolved Hide resolved
files/en-us/web/api/element/ariaatomic/index.html Outdated Show resolved Hide resolved
files/en-us/web/api/element/ariachecked/index.html Outdated Show resolved Hide resolved
files/en-us/web/api/element/ariareadonly/index.html Outdated Show resolved Hide resolved
files/en-us/web/api/element/ariarequired/index.html Outdated Show resolved Hide resolved
files/en-us/web/api/element/ariaroledescription/index.html Outdated Show resolved Hide resolved
files/en-us/web/api/element/ariavaluemax/index.html Outdated Show resolved Hide resolved
files/en-us/web/api/element/index.html Outdated Show resolved Hide resolved
@rachelandrew
Copy link
Collaborator Author

@jpmedley there are a couple of things in the comments but otherwise most of this is done.

Copy link
Collaborator

@jpmedley jpmedley left a comment

Choose a reason for hiding this comment

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

I should have set some expectations up front. I've rarely seen a PR this large that didn't have require multiple requests for changes. I've never even pulled that off myself. Thank you for your perseverance.

files/en-us/web/api/element/ariaatomic/index.html Outdated Show resolved Hide resolved
files/en-us/web/api/element/ariachecked/index.html Outdated Show resolved Hide resolved
files/en-us/web/api/element/ariaatomic/index.html Outdated Show resolved Hide resolved
files/en-us/web/api/element/ariahidden/index.html Outdated Show resolved Hide resolved
files/en-us/web/api/element/ariadisabled/index.html Outdated Show resolved Hide resolved
@rachelandrew
Copy link
Collaborator Author

@jpmedley I've made the requested changes and fixed the conflict but I can't get github to let me re-request review (I did get git into a bit of a mess with this one).

@jpmedley
Copy link
Collaborator

@chrisdavidmills This is ready to merge, but we're blocked on BCD. Should we create a label for that?

@chrisdavidmills
Copy link
Contributor

@jpmedley so are you after something like "needs-bcd", which would cover general states of PRs for new pages that haven't got BCD available yet, or do you want something more specific than that, which relates to BCD being submitted but not yet merged?

And do you think this label should block a content PR being merged? I have mixed feelings about this — I think it is fine to have content available without the corresponding BCD as long as it doesn't spend too long like that, and I feel that if BCD is really complex to unravel and figure out, it would be bad for the page content to be blocked on appearing because of that.

@Ryuno-Ki
Copy link
Collaborator

And do you think this label should block a content PR being merged?

From what I observed, in these cases, the BCD table would be simply non-existent.
If we want to have the label not block a content PR, then Yari should show some kind of hint, that no data was found.

@jpmedley
Copy link
Collaborator

@chrisdavidmills I had mixed feelings about making the request.

Here's the problem I'm trying to solve. Until now, I've mostly done new APIs, which means the BCD work is simple. Version numbers for Chrome and Edge. False for everybody else. Now me and the folks working with me are doing more backlog work, which means other browsers often don't have false for a value. I don't have a way to get that data, so it takes longer for the BCD PRs to get reviewed and merged. This means the possible time that an API can live without a BCD can be longer, increasing the likelihood of someone submitting duplicate work.

@chrisdavidmills
Copy link
Contributor

chrisdavidmills commented Feb 25, 2021

@jpmedley Understood. I think that in these cases it might be worth working out a procedure for trying to get the missing data in, e.g.

  • you start to work on documenting feature a
  • you submit a PR for the BCD for feature a
  • you @-mention some folks to help you find data for the bits you can't get hold of so easily (we'd have to work out who these are)
  • these folks help fill in the data
  • The BCD is merged.

I still reckon it is worth having a label to mark work that has this issue. I'd go for something like "bcd-incomplete", and then say that if the data is not found within a week, we should merge the BCD anyway, with null values, then open up a new issue in the BCD repo saying that this data is missing and inviting people to help?

@jpmedley jpmedley merged commit 2f54395 into mdn:main Feb 25, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants