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

Add comma separator to thousands place in title #124

Merged
merged 1 commit into from
Mar 23, 2018
Merged

Conversation

eawoods
Copy link
Contributor

@eawoods eawoods commented Mar 23, 2018

Screenshot taken doing testing on metadata.total+1000, since my test dataset doesn't have > 1,000 records.
screen shot 2018-03-23 at 11 28 54 am
Plus opportunistic adjustments to help text (added author bits that will be included with ARXIVNG-372, and applied help tips from advanced to simple search for parity).

@coveralls
Copy link

Coverage Status

Coverage remained the same at 82.122% when pulling e261e2e on task/ARXIVNG-384 into bf3aa74 on master.

@mhl10
Copy link
Contributor

mhl10 commented Mar 23, 2018

Looks good! Do you think the hit number should also include the separator? I don't feel strongly either way.
screenshot 2018-03-23 14 11 49

Copy link
Contributor

@erickpeirson erickpeirson left a comment

Choose a reason for hiding this comment

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

This looks great to me. But the PR should be to develop branch, not master.

@eawoods
Copy link
Contributor Author

eawoods commented Mar 23, 2018

OMG I can't believe I did that! It's been awhile....sigh. @erickpeirson nice catch....has been changed.

@eawoods eawoods changed the base branch from master to develop March 23, 2018 18:54
@eawoods
Copy link
Contributor Author

eawoods commented Mar 23, 2018

@mhl10 oo, good question. I am thinking not to add the comma for the numbered list placement - but that's a mild opinion. @erickpeirson what do you think?

@erickpeirson
Copy link
Contributor

I think leave as is

@eawoods eawoods merged commit 3715563 into develop Mar 23, 2018
@eawoods eawoods deleted the task/ARXIVNG-384 branch March 23, 2018 19:05
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.

4 participants