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

Prefix sample type menu entry ids with "sampletype-". #792

Merged
merged 1 commit into from
Aug 8, 2023

Conversation

ghemawat
Copy link
Contributor

@ghemawat ghemawat commented Aug 8, 2023

Previously, we would use the sample type name as the HTML id for the corresponding menu entry. However some profiles contain sample type names that conflict with other HTML ids we use (in particular, the sample type name "sample" conflicted with a different id). Remove this name clash by prefixing every menu entry with "sampletype-".

@ghemawat ghemawat requested a review from Louis-Ye August 8, 2023 20:45
@codecov-commenter
Copy link

codecov-commenter commented Aug 8, 2023

Codecov Report

Merging #792 (f2511f9) into main (2ba5b33) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #792   +/-   ##
=======================================
  Coverage   66.99%   66.99%           
=======================================
  Files          45       45           
  Lines        9862     9862           
=======================================
  Hits         6607     6607           
  Misses       2795     2795           
  Partials      460      460           

Louis-Ye
Louis-Ye previously approved these changes Aug 8, 2023
Copy link
Collaborator

@Louis-Ye Louis-Ye left a comment

Choose a reason for hiding this comment

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

LGTM, with a minor comment.

function setSampleIndexLink(id) {
const elem = document.getElementById(id);
function setSampleIndexLink(si) {
const elem = document.getElementById('sampletype-'+si);
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: I think the grammar convention in this file is having space before/after the operator +. Can you change it to that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Thanks.

Previously, we would use the sample type name as the HTML id for the
corresponding menu entry. However some profiles contain sample type
names that conflict with other HTML ids we use (in particular, the
sample type name "sample" conflicted with a different id). Remove this
name clash by prefixing every menu entry with "sampletype-".

some profiles caused the Javascript to break bec
Copy link
Collaborator

@Louis-Ye Louis-Ye left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@Louis-Ye Louis-Ye merged commit 4887780 into google:main Aug 8, 2023
49 checks passed
@ghemawat ghemawat deleted the sample branch August 8, 2023 22:37
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.

3 participants