-
-
Notifications
You must be signed in to change notification settings - Fork 114
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
Fix capitalisation and punctuation in report #796
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I leave small changes request bellow
public/js/hit.js
Outdated
|
||
if (this.props.showQueryCrumbs && this.props.showHitCrumbs) { | ||
// Multiper queries, multiple hits | ||
meta = `hit ${this.props.hit.number} of query ${this.props.query.number}, ` + meta; | ||
meta = `hit ${this.props.hit.number} of query ${this.props.query.number}. ` + meta; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@3lviend let's make string concatenation consistent,
'hit ${this.props.hit.number} of query ${this.props.query.number}. ' + meta;
to
'hit ${this.props.hit.number} of query ${this.props.query.number}. ${meta}';
public/js/query.js
Outdated
var length = `length: ${this.queryLength().toLocaleString()}`; | ||
var meta = length.charAt(0).toUpperCase() + length.slice(1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is chartAt(0)
is l
char in above assignment?
var meta = 'Length: ${this.queryLength().toLocaleString()}'
should be fixed, right?
or did I miss something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's right, just make it simpler with length
-> Length
. I'll update it 🚀
I thought before, we need dynamic code, without live updates to Length
or anything, so I made it like now, so that later whatever the sentence is will always be capitalized.
Issue Capitalisation and punctuation in report
Result: