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

Editorial: Pivots the tables within clause 16.2.1.5.4 #3342

Merged
merged 1 commit into from
Jul 17, 2024

Conversation

gesa
Copy link
Member

@gesa gesa commented May 31, 2024

This provides improved readability while maintaining the same information.

I will also be making these changes to the ever-so-slightly different 2024 branch, but i will likely not open a pr on that branch as that work’s mostly for me.

@gesa gesa force-pushed the pivot-some-tables branch 2 times, most recently from ff764db to 78bc529 Compare May 31, 2024 20:21
</tr>
<tr>
<th>[[PendingAsyncDependencies]]</th>
<td>1 (_B_)</td>
<th>0</th>
Copy link
Member Author

Choose a reason for hiding this comment

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

Is this a <th> on purpose? This is what it looks like on the spec.
2405_lxgf8

Copy link
Member

Choose a reason for hiding this comment

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

seems like a typo (weird that the ]] isn't sticking together on the same line, btw)

Copy link
Contributor

Choose a reason for hiding this comment

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

Almost certainly 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.

@ljharb outside my focus, but fwiw that’s what it looks like in firefox. Here’s some examples of table 45 in 16.2.1.5.4

Safari:
2406_0qawe

Chrome:
2406_0il40

Firefox:
2406_ddlnc

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah that th is for sure a typo.

@syg
Copy link
Contributor

syg commented Jun 5, 2024

Now that I see the "Field / Module" test, I don't actually like how it looks.

What do people think of the following:

          <emu-table id="table-module-graph-cycle-async-fields-7" caption="Module fields after module _C_ finishes with an error">
            <table>
              <col>
              <col span="2"></colgroup>
              <thead>
                <tr>
                  <td></td>
                  <th colspan="2" scope="colgroup">Module</th>
                </tr>
                <tr>
                  <td></td>
                  <th scope="col">_A_</th>
                  <th scope="col">_C_</th>
                </tr>
              </thead>
              <tr>
                <th scope="row">[[DFSIndex]]</th>
                <td>0</td>
                <td>3</td>
              </tr>
              <tr>
                <th scope="row">[[DFSAncestorIndex]]</th>
                <td>0</td>
                <td>0</td>
              </tr>
              <tr>
                <th scope="row">[[Status]]</th>
                <td>~evaluated~</td>
                <td>~evaluated~</td>
              </tr>
              <tr>
                <th scope="row">[[AsyncEvaluation]]</th>
                <td>*true*</td>
                <td>*true*</td>
              </tr>
              <tr>
                <th scope="row">[[AsyncParentModules]]</th>
                <td>« »</td>
                <td>« _A_ »</td>
              </tr>
              <tr>
                <th scope="row">[[PendingAsyncDependencies]]</th>
                <td>1 (_B_)</td>
                <th>0</th>
              </tr>
              <tr>
                <th scope="row">[[EvaluationError]]</th>
                <td>~empty~</td>
                <td>_C_'s evaluation error</td>
              </tr>
            </table>
          </emu-table>

Screenshot from 2024-06-05 12-16-36

Copy link
Contributor

@syg syg left a comment

Choose a reason for hiding this comment

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

I've reviewed the pivoted tables for correctness and everything looks good.

Some comments on formatting:

  • I don't like how "Fields / Modules" looks after all. See comment for an alternative.
  • The field names should be left-aligned instead of centered.
  • The <th> tags should have scope="row" or scope="col".

@bakkot
Copy link
Contributor

bakkot commented Jun 5, 2024

@syg's suggestion works for me. Alternatively, something like this:

Screenshot 2024-06-05 at 3 07 25 PM

code from chatgpt
<!DOCTYPE html>
<html lang="en">
<head>
    <meta charset="UTF-8">
    <meta name="viewport" content="width=device-width, initial-scale=1.0">
    <style>
        table {
            border-collapse: collapse;
            width: 100%;
        }
        th, td {
            border: 1px solid #000;
            padding: 8px;
            text-align: center;
        }
        .corner-cell {
            position: relative;
            height: 50px; /* Adjust as needed */
            width: 100px; /* Adjust as needed */
        }
        .corner-cell .slash {
            position: absolute;
            top: 0;
            left: 0;
            width: 100%;
            height: 100%;
            background: linear-gradient(to bottom left, transparent calc(50% - 1px), black, transparent calc(50% + 1px));
        }
        .corner-cell .fields {
            position: absolute;
            bottom: 2px;
            left: 5px;
            z-index: 1;
            background-color: white; /* Optional, to improve readability */
        }
        .corner-cell .programs {
            position: absolute;
            top: 2px;
            right: 5px;
            z-index: 1;
            background-color: white; /* Optional, to improve readability */
        }
    </style>
</head>
<body>
    <table>
        <tr>
            <th class="corner-cell">
                <div class="slash"></div>
                <span class="fields">Fields</span>
                <span class="programs">Modules</span>
            </th>
            <th>A</th>
            <th>B</th>
            <!-- Add more columns as needed -->
        </tr>
        <tr>
            <th>[[DFSIndex]]</th>
            <td>data1</td>
            <td>data2</td>
            <!-- Add more cells as needed -->
        </tr>
        <tr>
            <th>[[DFSAncestorIndex]]</th>
            <td>data3</td>
            <td>data4</td>
            <!-- Add more cells as needed -->
        </tr>
        <!-- Add more rows as needed -->
    </table>
</body>
</html>

@syg
Copy link
Contributor

syg commented Jun 5, 2024

@bakkot's snippet is my preference.

@michaelficarra
Copy link
Member

@bakkot I'm fine with something like that, but the padding needs to be adjusted so we don't have the labels shoved in the corners.

@michaelficarra
Copy link
Member

Added the slashed corner header cell. It looks like this:

image

@bakkot
Copy link
Contributor

bakkot commented Jun 27, 2024

Looks great.

spec.html Outdated Show resolved Hide resolved
@michaelficarra michaelficarra added the editor call to be discussed in the next editor call label Jul 1, 2024
@michaelficarra
Copy link
Member

@syg Can you take a look at the screenshot I posted and let us know if this is ready to merge?

@michaelficarra michaelficarra removed the editor call to be discussed in the next editor call label Jul 3, 2024
@michaelficarra michaelficarra added the ready to merge Editors believe this PR needs no further reviews, and is ready to land. label Jul 17, 2024
This provides improved readability while maintaining the same information.
@ljharb ljharb merged commit e0c4006 into tc39:main Jul 17, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge Editors believe this PR needs no further reviews, and is ready to land.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants