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

Format node/relationship counts in sidebar, inspector and legend #1008

Merged
merged 6 commits into from
Dec 6, 2019

Conversation

jk05
Copy link

@jk05 jk05 commented Dec 4, 2019

fixes: #953

  • Adds in thousands comma separators for numbers (shown in screenshot)
  • Naming suggestions very welcome as Im not sure what to call the util
  • Ive called it mostly inline but happy to go with assigning it to a variable first for readability, depending on how we do things in Neo4j

Screenshot 2019-12-04 at 10 05 48

@@ -64,7 +65,7 @@ const createItems = (
if (showStar) {
let str = '*'
if (count) {
str = `${str}(${count})`
str = `${str}(${thousandsCommaSeparatedNumber(count)})`
Copy link
Author

Choose a reason for hiding this comment

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

I did this here as I assume we would always want to comma separate whenever createItems is called...

*
*/

export default value =>
Copy link
Author

Choose a reason for hiding this comment

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

Im going to have another look at this, but the general thought process is for defensive purposes but I dont know how defensive we need to be here

  • parseInt to coerce everything to numbers, but need to do the isNaN check for null etc

It reads a bit odd anyway but open to suggestions, I will take another look in a bit

…nd rework the function with logical operators
@jk05 jk05 assigned jk05 and unassigned jk05 Dec 5, 2019
Copy link
Member

@oskarhane oskarhane left a comment

Choose a reason for hiding this comment

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

Looks good! Just want one or two more tests and then 👍

src/shared/utils/number-to-US-locale.js Outdated Show resolved Hide resolved
src/shared/utils/number-to-US-locale.test.js Outdated Show resolved Hide resolved
@jk05 jk05 changed the title commas-node-and-relation-count-new: initial Add util function to add thousands comma separated strings for node and relationship values Dec 5, 2019
@jk05 jk05 changed the title Add util function to add thousands comma separated strings for node and relationship values Format node/relationship counts in sidebar, inspector and legend Dec 5, 2019
Jas Kuner added 2 commits December 5, 2019 15:25
…t path, added additional tests and reworked number-to-US-locale.js to allow 0 to be output as string to be consistent with other values
@jk05 jk05 requested a review from oskarhane December 6, 2019 09:34
Copy link
Member

@oskarhane oskarhane left a comment

Choose a reason for hiding this comment

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

LGTM 🎉

@jk05 jk05 merged commit eac0ed1 into neo4j:master Dec 6, 2019
@jk05 jk05 deleted the commas-node-and-relation-count-new branch December 6, 2019 10:55
@jexp
Copy link
Member

jexp commented Feb 3, 2020

would it be possible to make the formatting rendering only? so that when someone copies the number it's just the raw number?

also it would be amazing to add the counts to the other labels and rel-types too

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

Successfully merging this pull request may close these issues.

Request: Commas in node and relation count
3 participants