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

render blob in code view when given a line number #233

Merged
merged 5 commits into from Oct 5, 2018

Conversation

Projects
None yet
4 participants
@vanesa
Member

vanesa commented Oct 2, 2018

render raw on any file type in code view when url includes a line number and does not explicitly ask for rendered

Fix sourcegraph/enterprise#11991

This PR updates the CHANGELOG.md file to describe any user-facing changes.

@vanesa vanesa requested a review from felixfbecker as a code owner Oct 2, 2018

@@ -190,7 +190,11 @@ export class BlobPage extends React.PureComponent<Props, State> {
const renderMode = ToggleRenderedFileMode.getModeFromURL(this.props.location)
// renderAs is renderMode but with undefined mapped to the actual mode.
const renderAs = renderMode || (this.state.blobOrError && this.state.blobOrError.richHTML ? 'rendered' : 'code')
const renderAs =
renderMode ||

This comment has been minimized.

@sqs

sqs Oct 2, 2018

Member

It would be helpful to add a comment explaining the desired behavior for this line since it is now somewhat nuanced

@sqs

sqs Oct 2, 2018

Member

It would be helpful to add a comment explaining the desired behavior for this line since it is now somewhat nuanced

This comment has been minimized.

@sqs

sqs Oct 3, 2018

Member

I saw your added comment. I think it would be beneficial for the comment to be more descriptive and explain the specific cases this handles, listed in bullet items, and the why. Otherwise I think someone would easily change this and not realize they were causing a regression. One test would be to show just the comment and line of code to another dev, and see if they can repeat back to you the desired behavior and why.

This would also be a great candidate for unit testing (but that needn't block this PR). cc @felixfbecker to think about how to encourage unit testing of these things

@sqs

sqs Oct 3, 2018

Member

I saw your added comment. I think it would be beneficial for the comment to be more descriptive and explain the specific cases this handles, listed in bullet items, and the why. Otherwise I think someone would easily change this and not realize they were causing a regression. One test would be to show just the comment and line of code to another dev, and see if they can repeat back to you the desired behavior and why.

This would also be a great candidate for unit testing (but that needn't block this PR). cc @felixfbecker to think about how to encourage unit testing of these things

@sqs

This comment has been minimized.

Show comment
Hide comment
@sqs

sqs Oct 2, 2018

Member

Should be:

render raw (not markdown) on any file type in code view when url includes a line number and does not explicitly ask for rendered

Member

sqs commented Oct 2, 2018

Should be:

render raw (not markdown) on any file type in code view when url includes a line number and does not explicitly ask for rendered

Show outdated Hide outdated src/repo/blob/BlobPage.tsx Outdated

@vanesa vanesa requested review from ijsnow and slimsag Oct 4, 2018

// - If file does not contain richHTML or the url includes a line number: We render in code view.
const renderAs =
renderMode ||
(this.state.blobOrError && this.state.blobOrError.richHTML && !parseHash(this.props.location.hash).line

This comment has been minimized.

@felixfbecker

felixfbecker Oct 5, 2018

Member

Line zero should also jump to the code view

@felixfbecker

felixfbecker Oct 5, 2018

Member

Line zero should also jump to the code view

This comment has been minimized.

@vanesa

vanesa Oct 5, 2018

Member

Line 0? In which scenario would there be a line 0?

@vanesa

vanesa Oct 5, 2018

Member

Line 0? In which scenario would there be a line 0?

This comment has been minimized.

@felixfbecker

felixfbecker Oct 5, 2018

Member

You can link to line 0 in the URL. Zero is falsy, so your check !parseHash(this.props.location.hash).line would be true, while it is false for any other line number like 123.

@felixfbecker

felixfbecker Oct 5, 2018

Member

You can link to line 0 in the URL. Zero is falsy, so your check !parseHash(this.props.location.hash).line would be true, while it is false for any other line number like 123.

This comment has been minimized.

@felixfbecker

felixfbecker Oct 5, 2018

Member

Actually, just realised that parseHash returns a 1-based line number, so what I said is not true.

@felixfbecker

felixfbecker Oct 5, 2018

Member

Actually, just realised that parseHash returns a 1-based line number, so what I said is not true.

This comment has been minimized.

@vanesa

vanesa Oct 5, 2018

Member

So I can merge? :)

@vanesa

vanesa Oct 5, 2018

Member

So I can merge? :)

@vanesa vanesa removed the request for review from slimsag Oct 5, 2018

@vanesa vanesa merged commit d653a7c into master Oct 5, 2018

2 checks passed

buildkite/sourcegraph Build #21415 passed (4 minutes, 52 seconds)
Details
cla-bot Contributor has signed the CLA

@vanesa vanesa deleted the vo/markdown-file-fix branch Oct 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment