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

Wrap line numbers in spans #28

Closed

Conversation

twe4ked
Copy link
Contributor

@twe4ked twe4ked commented Oct 3, 2016

I also added a commit to exclude a trailing blank newline from the line numbers.

@@ -43,11 +43,11 @@ function createElement({ node, style, useInlineStyles, key }) {
}

function getLineNumberString(lines, startingLineNumber) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe we should change this to just getLineNumbers now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, missed this. Good catch.

@@ -43,11 +43,11 @@ function createElement({ node, style, useInlineStyles, key }) {
}

function getLineNumberString(lines, startingLineNumber) {
return lines.reduce((lineCountString, _, i) => lineCountString + `${i + startingLineNumber}\n`, '');
return lines.map((_line, i) => { return <span className="line" key={"line-" + i}>{`${i + startingLineNumber}\n`}</span> })
Copy link
Collaborator

@conorhastings conorhastings Oct 3, 2016

Choose a reason for hiding this comment

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

can we use single quotes on the className and a template string for key

key={`line-${i}`}

Copy link
Collaborator

Choose a reason for hiding this comment

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

The first argument here can just be a _ as well

Copy link
Collaborator

Choose a reason for hiding this comment

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

And for the className can we use something more unique, maybe scope it to the library?
className='react-syntax-highlighter-line-number

}

function LineNumbers({ codeString, style = {float: 'left', paddingRight: '10px'}, startingLineNumber }) {
return <code style={style}>{getLineNumberString(codeString.split('\n'), startingLineNumber)}</code>
return <code style={style}>{getLineNumberString(codeString.replace(/\n$/, '').split('\n'), startingLineNumber)}</code>
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the replace for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So we don't get a line number for a trailing newline. See: 05df3b8

Copy link
Collaborator

Choose a reason for hiding this comment

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

cool

@conorhastings conorhastings self-assigned this Oct 3, 2016
function getLineNumberString(lines, startingLineNumber) {
return lines.reduce((lineCountString, _, i) => lineCountString + `${i + startingLineNumber}\n`, '');
function getLineNumbers(lines, startingLineNumber) {
return lines.map((_, i) => { return <span className="react-syntax-highlighter-line-number" key={`line-${i}`}>{`${i + startingLineNumber}\n`}</span> })
Copy link
Collaborator

Choose a reason for hiding this comment

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

we can use implicit return here instead of { and explcit return statement

@conorhastings
Copy link
Collaborator

@twe4ked that last comment + update the tests and it looks good! thanks!

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.

2 participants