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.

Sign up for GitHub

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

Jump to bottom

Remove d3 format option from PopUps #68

Merged
merged 6 commits into from
Jul 9, 2020
Merged

Remove d3 format option from PopUps #68

merged 6 commits into from
Jul 9, 2020

Conversation

VictorVelarde
Copy link
Contributor

This is a small PR to remove the dependency on d3.format for the library.

Main reasons:

  • I think it doesn't provide much value and it adds a dependency.
  • you can get exactly the same results importing it externally, from your project.

So instead of

   {
      attr: 'pop',
      title: 'Population D3',
      format: '~s'
 }

the user would use the general option to pass any function

   {
      attr: 'pop',
      title: 'Population D3',
      format: d3.format('~s')
 }

As a complement:

  • This will also work in the future when we consider date fields (the user might use externally for example moment.js)
  • We save some KBs (just a few).

@VictorVelarde VictorVelarde requested a review from manmorjim July 7, 2020 09:53
@vercel
Copy link

vercel bot commented Jul 7, 2020 •
edited
Loading

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

@VictorVelarde
Copy link
Contributor Author

/cc before entering into CR, @borja-munoz do you agree on this? I think it's a good moment to remove this.

@VictorVelarde VictorVelarde marked this pull request as ready for review July 7, 2020 14:13
@VictorVelarde VictorVelarde marked this pull request as draft July 7, 2020 14:14
@VictorVelarde
Copy link
Contributor Author

It looks like we have some failing test, related to histogram @jesusbotella

@jesusbotella
Copy link
Contributor

I see that tests are passing right now. Did you re-run them?

@VictorVelarde
Copy link
Contributor Author

GITHUB issue? I'm not sure why, but it looks like here we have some comments that belongs to another PR (at least in my browser I get some un-related comments about 'histogram', that don't belong to this).

@jesusbotella
Copy link
Contributor

Yes, not sure. I saw that histogram comment as well 😅 , but tests are passing.

@VictorVelarde
Copy link
Contributor Author

I ran them again, yes!

@VictorVelarde VictorVelarde force-pushed the feature/remove-d3-format branch from e3fdbc6 to 0368fbc Compare July 8, 2020 09:49
@VictorVelarde
Copy link
Contributor Author

.... ok, so coming back to this issue.... Please @manmorjim have a look at it

@VictorVelarde VictorVelarde marked this pull request as ready for review July 8, 2020 09:55
manmorjim
manmorjim suggested changes Jul 8, 2020
View reviewed changes
Copy link
Contributor

@manmorjim manmorjim left a comment

Choose a reason for hiding this comment

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

Everything looks great to me 👌

Just a small request to keep the format options for custom functions.

src/lib/viz/popups/Popup.ts
/**
* d3 format for the value of this attribute.
*/
format?: string | FormatFunction;
Copy link
Contributor

Choose a reason for hiding this comment

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

we should keep the format option for functions right?

format?: (value: any) => any;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought it was just related to d3... oka

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup sorry, the comment was outdated and wrong 😅

src/lib/viz/popups/Popup.ts Outdated
}

elementValue = formatter(elementValue);
} else if (format && typeof format !== 'function') {
Copy link
Contributor

Choose a reason for hiding this comment

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

the second condition is not necessary

...
} else if (format) {
...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

for sure! thx

@VictorVelarde VictorVelarde force-pushed the feature/remove-d3-format branch from 275a5f4 to ff306ae Compare July 8, 2020 19:18
@VictorVelarde
Copy link
Contributor Author

Rebasing to get changes from develop...

manmorjim
manmorjim approved these changes Jul 9, 2020
View reviewed changes
Copy link
Contributor

@manmorjim manmorjim left a comment

Choose a reason for hiding this comment

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

🚀

@VictorVelarde VictorVelarde force-pushed the feature/remove-d3-format branch from ef48d6e to 269af53 Compare July 9, 2020 08:38
@VictorVelarde VictorVelarde merged commit d429512 into develop Jul 9, 2020
@VictorVelarde VictorVelarde deleted the feature/remove-d3-format branch July 9, 2020 08:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@manmorjim manmorjim manmorjim approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@VictorVelarde @jesusbotella @manmorjim