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

fix: KeyboardLayoutMap.keys/values/entries should have methods type #29957

Merged
merged 19 commits into from
Dec 18, 2023

Conversation

skyclouds2001
Copy link
Contributor

@skyclouds2001 skyclouds2001 commented Nov 1, 2023

Description

Motivation

note that KeyboardLayoutMap is a map-like object, see https://wicg.github.io/keyboard-map/#keyboardlayoutmap-interface, and KeyboardLayoutMap.entries() KeyboardLayoutMap.keys() KeyboardLayoutMap.values() are methods instead of properties

a example is below

navigator.keyboard.getLayoutMap().then((layout) => {
  for (const l of layout.entries()) {
    console.log(l)
  }
})

Additional details

Working on https://developer.mozilla.org/en-US/docs/Web/API/KeyboardLayoutMap
Related with https://wicg.github.io/keyboard-map/

Related issues and pull requests

Relates to #6891

@github-actions github-actions bot added the Content:WebAPI Web API docs label Nov 1, 2023
@skyclouds2001 skyclouds2001 changed the title fix KeyboardLayoutMap fix KeyboardLayoutMap keys values entries type Nov 1, 2023
@skyclouds2001 skyclouds2001 marked this pull request as ready for review November 1, 2023 13:18
@skyclouds2001 skyclouds2001 requested a review from a team as a code owner November 1, 2023 13:18
@skyclouds2001 skyclouds2001 requested review from sideshowbarker and removed request for a team November 1, 2023 13:18
Copy link
Collaborator

@wbamberg wbamberg left a comment

Choose a reason for hiding this comment

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

Thank you for the PR, @skyclouds2001 ! But there's more work needed in here. Changing the page type is good, but the page itself is written as if this is a property (e.g. it's described as a property, it doesn't have "Parameters" or "Return value", etc.).

Note that it's a bit unsettled how we should document members of maplike and setlike interfaces: sometimes we document the methods and properties fully (e.g. https://developer.mozilla.org/en-US/docs/Web/API/RTCStatsReport/entries), sometimes we just list them in the interface main page and link to the Map/Set object (e.g. https://developer.mozilla.org/en-US/docs/Web/API/EventCounts).

openwebdocs/project#159 is supposed to help resolve this but it's not ready yet.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/map#map-like_browser_apis is also a helpful page to link to in places like this :).

@sideshowbarker sideshowbarker removed their request for review November 2, 2023 05:13
@skyclouds2001

This comment was marked as duplicate.

@skyclouds2001 skyclouds2001 changed the title fix KeyboardLayoutMap keys values entries type fix: KeyboardLayoutMap.keys/values/entries should have methods type Dec 9, 2023
Copy link
Contributor

@teoli2003 teoli2003 left a comment

Choose a reason for hiding this comment

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

@skyclouds2001 This is a great catch and essential work, long due! Thank you!

As this PR defines the page structure for these maplike members, it is essential to take some time to carefully review them, as these structures will be reused for the 5-6 other such interfaces (that need them). That way, we won't do this work each time.

A few additions that I would like to see:

  • Adding a spec-url entry in the FrontMatter YAML, like for Hightlight.entries()Hightlight is a set-like object – that links to the Set.entries() definition. Same for all other pages, to their relevant definitions, of course. Ultimately, we should store it in BCD, but I don't see a need to couple both discussions – I know there is no consensus between BCD peers 😉 – and hold this PR more than necessary.
  • An example for each method. They will be similar, except in how the object is created. Highlight's methods have most of them written and are a good place to start.

Once this PR is merged, I think we should update the meta-documentation for these setlike and maplike pages, so we can refer writers when new such interfaces have to be added.

I'm really excited about this work. Thanks a lot for tackling this.

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Copy link
Contributor

@teoli2003 teoli2003 left a comment

Choose a reason for hiding this comment

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

This looks good to me. @skyclouds2001 Do you want to update the other set-like and map-like interfaces with similar pages?

If you want, I can prepare an issue with the list of such interfaces.

@teoli2003 teoli2003 merged commit 33d8f83 into mdn:main Dec 18, 2023
7 checks passed
@skyclouds2001 skyclouds2001 deleted the KeyboardLayoutMap branch December 18, 2023 11:03
@skyclouds2001
Copy link
Contributor Author

This looks good to me. @skyclouds2001 Do you want to update the other set-like and map-like interfaces with similar pages?

If you want, I can prepare an issue with the list of such interfaces.

I'd love to do it!

dipikabh pushed a commit to dipikabh/content that referenced this pull request Jan 17, 2024
…dn#29957)

* update

* update

* fix readonly

* update summary

* update

* update

* add link

* fix title

* feat: update

* Update files/en-us/web/api/keyboardlayoutmap/entries/index.md

* add content for @@iterator

* update

* fix

* specific spec-urls

* fix word

* add example section

* Apply suggestions from code review

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

* Update files/en-us/web/api/keyboardlayoutmap/has/index.md

---------

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
Content:WebAPI Web API docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants