Skip to content

re-arrange option tables#16083

Merged
seanbudd merged 2 commits intobetafrom
fixOptionTables
Jan 24, 2024
Merged

re-arrange option tables#16083
seanbudd merged 2 commits intobetafrom
fixOptionTables

Conversation

@seanbudd
Copy link
Copy Markdown
Member

@seanbudd seanbudd commented Jan 23, 2024

Link to issue number:

Fixes #16063

Summary of the issue:

#15950 converted definition lists to tables, however the conversion could have been done better.
Both lines of the table start with header syntax || whereas it should only be the first header.

Tables were previously converted from definition lists to look like this

Default Enabled
Options Default (Enabled), Enabled, Disabled

Options should be listed before default logically.
The first column should be headers, not the first row.
Unfortunately there is no good support for column headers in markdown in our markdown parsing stack.
As such, this can be improved by just having no headers.

Description of user facing changes

Change all option definition tables to be formatted as follows.
The header row is hidden in CSS

Options Default (Enabled), Enabled, Disabled
Default Enabled

Description of development approach

The following regex was used:

  • find: \|\|(.*)\|(.*)\|\n\|\|(.*)\|(.*)\|
  • replace: || . {.hideHeaderRow} | . |\n|$3|$4|\n|$1|$2|

Testing strategy:

Confirmed by building user docs and checking output

Known issues with pull request:

None

Code Review Checklist:

  • Documentation:
    • Change log entry
    • User Documentation
    • Developer / Technical Documentation
    • Context sensitive help for GUI changes
  • Testing:
    • Unit tests
    • System (end to end) tests
    • Manual testing
  • UX of all users considered:
    • Speech
    • Braille
    • Low Vision
    • Different web browsers
    • Localization in other languages / culture than English
  • API is compatible with existing add-ons.
  • Security precautions taken.

@seanbudd seanbudd requested review from a team as code owners January 23, 2024 01:53
@seanbudd seanbudd requested review from Qchristensen and michaelDCurran and removed request for a team January 23, 2024 01:54
@seanbudd seanbudd added this to the 2024.1 milestone Jan 23, 2024
@wmhn1872265132
Copy link
Copy Markdown
Contributor

It is recommended to use the order of | Default |Options|. The first impression after reading the current order is that Default is the value of Options rather than the header.

@cary-rowen
Copy link
Copy Markdown
Contributor

I also agree with @wmhn1872265132 that it might be better to replace the left and right column positions.

@seanbudd
Copy link
Copy Markdown
Member Author

seanbudd commented Jan 23, 2024

It is recommended to use the order of | Default |Options|. The first impression after reading the current order is that Default is the value of Options rather than the header.

Is this implication not an issue with these reversed? Wouldn't you get the impressions that "Options" is the value of "Default" rather than the headers.

@wmhn1872265132
Copy link
Copy Markdown
Contributor

I don't think so, because the default is more like one of the options, but there is rarely an option called an option

@cary-rowen
Copy link
Copy Markdown
Contributor

If you consider a linear reading order, that is, users do not use table navigation to read, then @wmhn1872265132's concerns should be valid.

Do we need to think about it, for similar options, do we really need to format it into a table? What are the advantages of using tables? Including the previous list style also seems to have caused some misunderstandings.

Before using the list style, did some users report that this was not stated clearly enough.

@CyrilleB79
Copy link
Copy Markdown
Contributor

I understand and share the concern of @cary-rowen and @wmhn1872265132 regarding reading order, with "Options" coming before "Default" when reading with downArrow (or SayAll by the way).

Though, it's more logical to have first the information of the available options and then the default one.

I would also add that using columns is not very comfortable since it forces the user to use table navigation to read this 2x2 table. Instead, using rows to store the informations, the user could use either table navigation or just downArrow or SayAll. Moreover, visually, the layout may be quite ugly if the number and names of the options takes too much space. At last, in other materials where options are described, I tink it's alwas done in row, not in column, so people are used to this visual layout.

So my recommendation would be to use a table with the information in rows if possible, i.e.:

OptionsOption 1, Option 2, Option 3
DefaultOption 2

And, nice to have if possible, with the first column set as rows headers.

If horizontal layout is not technically possible, just use list instead:

  • Options: Option 1, Option 2, Option 3
  • Default: Option 2

@seanbudd
Copy link
Copy Markdown
Member Author

Native markdown only supports one type of table: the first row is always column headers, there are no other headers.
The general workaround is to use raw HTML like you've done, but we'd prefer to avoid HTML is markdown docs.

We can try out an extended python markdown plugin like this one:
https://github.com/tableau-tables/python-tableau-parser

@seanbudd
Copy link
Copy Markdown
Member Author

seanbudd commented Jan 24, 2024

This extension doesn't seem to be maintained well.
Alternatively, we can create empty header rows, and hide them with CSS

Markdown table:

| . {.hideHeaderRow} | . |
|---|---|
| Options | Enabled, Disabled |
| Default | Disabled |

CSS

thead:has(tr > th.hideHeaderRow) {
  display: none;
}

HTML output:

<table>
<thead>
<tr>
<th class="hideHeaderRow">.</th>
<th>.</th>
</tr>
</thead>
<tr><td>Options</td><td>Option 1, Option 2, Option 3</td></tr>
<tr><td>Default</td><td>Option 2</td></tr>
</table>

Rendered HTML output (note no styling)

. .
OptionsOption 1, Option 2, Option 3
DefaultOption 2

NVDA Output - note row numbers are wrong

Row 2, Column 1, Options
Column 2, Enabled, Disabled
Row 3, Column 2, Default

# Allows TOC with [TOC]"
"markdown.extensions.toc",
# Allows setting attributes with {@id=foo}
"markdown.extensions.legacy_attrs",
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

this has been removed as extra contains abetter method of setting attributes, and this method with @ conflicts with mdx_gh_links for github username links.

@CyrilleB79
Copy link
Copy Markdown
Contributor

What will be the syntax?

I am a bit concerned that the translators will have to copy/paste table headers row in the md file, making a md file less easy to read. Or won't this header string appear at all if the documentation is split in CrowdIn? How do these tables behave when the .md file is provided and split for translation?

Alternatively, I wonder if the list is not just simpler. Have you considered replacing the table by a list as suggested in #16083 (comment)?

Also, @seanbudd, you wrote:

The general workaround is to use raw HTML like you've done, but we'd prefer to avoid HTML is markdown docs.

Fully agree with you. The table in my example was meant to illustrate the result I want to achieve in the HTML User Guide; the way these tables were produced are not to be considered in this example.

@michaelDCurran
Copy link
Copy Markdown
Member

michaelDCurran commented Jan 24, 2024 via email

@lukaszgo1
Copy link
Copy Markdown
Contributor

Alternatively, I wonder if the list is not just simpler. Have you considered replacing the table by a list as suggested in #16083 (comment)?

+1 to this.

@seanbudd seanbudd merged commit 8462296 into beta Jan 24, 2024
@seanbudd seanbudd deleted the fixOptionTables branch January 24, 2024 22:51
Adriani90 pushed a commit to Adriani90/nvda that referenced this pull request Mar 13, 2024
Fixes nvaccess#16063

Summary of the issue:
nvaccess#15950 converted definition lists to tables, however the conversion could have been done better.
Both lines of the table start with header syntax || whereas it should only be the first header.

Tables were previously converted from definition lists to look like this

Default	Enabled
Options	Default (Enabled), Enabled, Disabled
Options should be listed before default logically.
The first column should be headers, not the first row.
Unfortunately there is no good support for column headers in markdown in our markdown parsing stack.
As such, this can be improved by just having no headers.

Description of user facing changes
Change all option definition tables to be formatted as follows.
The header row is hidden in CSS

Options	Default (Enabled), Enabled, Disabled
Default	Enabled
Description of development approach
The following regex was used:

find: \|\|(.*)\|(.*)\|\n\|\|(.*)\|(.*)\|
replace: || . {.hideHeaderRow} | . |\n|$3|$4|\n|$1|$2|
Testing strategy:
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants