Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

[SG-43144]: Tables in code excerpts should use headers for line numbers #43389

Merged
merged 8 commits into from
Nov 3, 2022
Merged

[SG-43144]: Tables in code excerpts should use headers for line numbers #43389

merged 8 commits into from
Nov 3, 2022

Conversation

gitstart-sourcegraph
Copy link
Collaborator

@gitstart-sourcegraph gitstart-sourcegraph commented Oct 25, 2022 •
edited by sourcegraph-bot
Loading

Description

ArcToolkit says the tables used for search results are confusing because a11y tools can't tell if the table is tabular data or used for layout purposes. We need to clarify this.

Ref

SG Issue
Gitstart Ticket

Success criteria

The table should switch the line numbers from being <td> to <th>. This will clarify that this is indeed a table with tabular data. ArcToolkit should not show alerts about tables.

Test plan

  • The issue mentioned above should be fixed

App preview:

Check out the client app preview documentation to learn more.

@gitstart-sourcegraph gitstart-sourcegraph self-assigned this Oct 25, 2022
@cla-bot cla-bot bot added the cla-signed label Oct 25, 2022
@gitstart-sourcegraph gitstart-sourcegraph added gitstart Contract partner frontend-platform Issues related to our frontend platform, owned collectively by our frontend crew. labels Oct 25, 2022
@gitstart-sourcegraph gitstart-sourcegraph linked an issue Oct 25, 2022 that may be closed by this pull request
[Accessibility]: Tables in code excerpts should use headers for line numbers #43144
Open
4 tasks
@gitstart-sourcegraph gitstart-sourcegraph changed the title [SG-43144] [Accessibility]: Tables in code excerpts should use headers for line numbers [SG-43144]: Tables in code excerpts should use headers for line numbers Oct 25, 2022
@sg-e2e-regression-test-bob
Copy link

sg-e2e-regression-test-bob commented Oct 25, 2022 •
edited
Loading

Bundle size report 📦

Initial size Total size Async size Modules
0.00% (+0.02 kb) 0.00% (+0.04 kb) 0.00% (+0.02 kb) 0.00% (0)

Look at the Statoscope report for a full comparison between the commits 02452ce and b74554d or learn more.

Open explanation
  • Initial size is the size of the initial bundle (the one that is loaded when you open the page)
  • Total size is the size of the initial bundle + all the async loaded chunks
  • Async size is the size of all the async loaded chunks
  • Modules is the number of modules in the initial bundle

gitstart-sourcegraph
gitstart-sourcegraph commented Oct 25, 2022
View reviewed changes
Copy link
Collaborator Author

@gitstart-sourcegraph gitstart-sourcegraph left a comment

Choose a reason for hiding this comment

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

Hi @umpox @limitedmage
Since layout and html structure of the line-number and code has been formulated on the backend part of the codebase, want to confirm with you before making the change in the following files to update the HTML tag used.

  • cmd/frontend/internal/highlight/highlight.go
  • cmd/frontend/internal/highlight/html.go

Screenshot 2022-10-21 at 2 55 57 PM (1)

WDYT?

@limitedmage
Copy link
Contributor

Yes, looks like fixing this does require backend changes. Let's go ahead and do them. If you need help with this let me know.

@gitstart-sourcegraph gitstart-sourcegraph marked this pull request as ready for review October 28, 2022 05:45
@limitedmage
Copy link
Contributor

I'm running this locally but the line numbers still seem to be rendered as td for me?
image

@gitstart-sourcegraph gitstart-sourcegraph marked this pull request as draft October 30, 2022 18:15
@gitstart-sourcegraph gitstart-sourcegraph marked this pull request as ready for review October 31, 2022 15:34
@gitstart-sourcegraph
Copy link
Collaborator Author

@limitedmage Sorry, we missed the required changes in syntax-highlighter image, for testing this we have to build it to get the updated image locally and run sg start oss with it.

These are steps:

  • Run docker build -t sourcegraph/syntax-highlighter:local . in docker-images/syntax-highlighter directory (it would take much time)
  • In sg.config.yaml > syntax-highlighter > cmd change sourcegraph/syntax-highlighter:insiders to sourcegraph/syntax-highlighter:local
  • Run sg start oss

Could you please help to recheck?

@limitedmage
Copy link
Contributor

@camdencheek or @slimsag can you please review the backend code for this? Thanks!

camdencheek
camdencheek approved these changes Nov 1, 2022
View reviewed changes
Copy link
Member

@camdencheek camdencheek left a comment

Choose a reason for hiding this comment

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

Backend looks good to me 👍

@limitedmage
Copy link
Contributor

@gitstart-sourcegraph I'm running the change and it works great in search results, but it breaks line numbers in the blob view:
image

It looks like we have quite a few places where we look for td.line. I think these need to be updated to th.line to match this new layout.

@limitedmage
Copy link
Contributor

@gitstart-sourcegraph Looks like there are some regressions in the Percy tests, can you take a look?

@gitstart-sourcegraph
Copy link
Collaborator Author

@limitedmage we updates PRs to fix UI diffs. Could you please help to check if we missed anything else?

limitedmage
limitedmage approved these changes Nov 2, 2022
View reviewed changes
Copy link
Contributor

@limitedmage limitedmage 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 your thoroughness with this larger-than-expected change. I tested it locally and it looks good!

valerybugakov
valerybugakov approved these changes Nov 3, 2022
View reviewed changes
Copy link
Member

@valerybugakov valerybugakov left a comment

Choose a reason for hiding this comment

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

LGTM!

@gitstart-sourcegraph gitstart-sourcegraph merged commit 5e41e12 into main Nov 3, 2022
@gitstart-sourcegraph gitstart-sourcegraph deleted the contractors/SG-43144 branch November 3, 2022 04:23
ggilmore added a commit that referenced this pull request Nov 16, 2022
@ggilmore
Revert "[SG-43144]: Tables in code excerpts should use headers for li…
6060aac 
…ne numbers (#43389)"

This reverts commit 5e41e12.
ggilmore added a commit that referenced this pull request Nov 16, 2022
@ggilmore
Revert "[SG-43144]: Tables in code excerpts should use headers for li…
99e458a 
…ne numbers (#43389)" (#44514)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@limitedmage limitedmage limitedmage approved these changes

@valerybugakov valerybugakov valerybugakov approved these changes

@camdencheek camdencheek camdencheek approved these changes

@umpox umpox Awaiting requested review from umpox

@emidoots emidoots Awaiting requested review from emidoots

Assignees

@gitstart-sourcegraph gitstart-sourcegraph

Labels
cla-signed frontend-platform Issues related to our frontend platform, owned collectively by our frontend crew. gitstart Contract partner
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Accessibility]: Tables in code excerpts should use headers for line numbers
5 participants
@gitstart-sourcegraph @sg-e2e-regression-test-bob @limitedmage @valerybugakov @camdencheek