Skip to content

Editorial: Add <dfn> to well‑known symbols #1996

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jun 11, 2020
Merged

Editorial: Add <dfn> to well‑known symbols #1996

merged 1 commit into from
Jun 11, 2020

Conversation

ExE-Boss
Copy link
Contributor

@ExE-Boss ExE-Boss commented May 17, 2020 •
edited
Loading

@ExE-Boss ExE-Boss mentioned this pull request May 17, 2020
Merged
ljharb
ljharb approved these changes May 17, 2020
View reviewed changes
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

This is a nice usability improvement.

@ljharb ljharb requested review from michaelficarra, syg, bakkot and a team May 17, 2020 20:38
bakkot
bakkot requested changes May 25, 2020
View reviewed changes
Copy link
Contributor

@bakkot bakkot left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I think these are the wrong places to put the <dfn>s. The table of well-known symbols is what defines @@whatever; the Symbol.whatever sections are just using those terms, like any other part of the spec. So I think the definitions should instead be moved to said table.

@ExE-Boss ExE-Boss requested a review from bakkot May 26, 2020 18:14
@ExE-Boss ExE-Boss force-pushed the fix/add-dfn-to-symbol-properties branch from f06d26a to f212663 Compare May 26, 2020 18:23
@ExE-Boss
Copy link
Contributor Author

The downside of moving the <dfn>s to the table of well‑known symbols is that the symbol auto‑links now point to the section as a whole instead of the specific symbol.

@bakkot
Copy link
Contributor

bakkot commented May 26, 2020

I'm fine with that. For someone who doesn't know what the @@ notation means, the context in that section is useful. And it's easy to find the particular symbol you're looking for in the table.

@ExE-Boss
Copy link
Contributor Author

ExE-Boss commented May 26, 2020 •
edited
Loading

It’s possible to fix that by simply adding the id attribute to the <dfn> (or the enclosing <td>/<tr>) elements, though that seems to have weird interaction with the on‑hover toolbox.

@ExE-Boss ExE-Boss changed the title Editorial: Add <dfn> to well‑known symbols Editorial: Add <dfn> to well‑known symbols May 31, 2020
@ljharb ljharb self-assigned this Jun 11, 2020
@ljharb ljharb force-pushed the fix/add-dfn-to-symbol-properties branch from 1b0d853 to c509b6b Compare June 11, 2020 05:43
@ljharb ljharb merged commit c509b6b into tc39:master Jun 11, 2020
@ljharb ljharb deleted the fix/add-dfn-to-symbol-properties branch June 11, 2020 05:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ljharb ljharb ljharb approved these changes

@syg syg syg approved these changes

@michaelficarra michaelficarra michaelficarra approved these changes

@bakkot bakkot bakkot approved these changes

Assignees

@ljharb ljharb

Labels
editorial change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add <dfn>s for all the well-known symbols (and make sure they link)
5 participants
@ExE-Boss @bakkot @ljharb @syg @michaelficarra