Skip to content
This repository has been archived by the owner on Dec 10, 2021. It is now read-only.

Use factory to create D3 number formatter in @superset-ui/number-format #42

Merged
merged 1 commit into from
Nov 29, 2018

Conversation

kristw
Copy link
Contributor

@kristw kristw commented Nov 29, 2018

🏠 Internal

  • Instead of creating D3NumberFormatter as subclass of NumberFormatter with different constructor, use a factory pattern to create NumberFormat with the given d3 format string. This seems like a better API since D3NumberFormatter is not taking advantage of the inheritance and also defining a subclass that has quite different constructor from parent can be confusing. The factory function fits better with this context. time-format is also written in the factory style.

@kristw kristw requested a review from a team as a code owner November 29, 2018 19:59
@kristw kristw changed the title Switch @superset-ui/number-format to factory pattern Use factory to create D3 number formatter in @superset-ui/number-format Nov 29, 2018
@codecov
Copy link

codecov bot commented Nov 29, 2018

Codecov Report

Merging #42 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #42      +/-   ##
=========================================
- Coverage   99.11%   99.1%   -0.01%     
=========================================
  Files          48      48              
  Lines         450     449       -1     
  Branches       39      39              
=========================================
- Hits          446     445       -1     
  Misses          2       2              
  Partials        2       2
Impacted Files Coverage Δ
...et-ui-number-format/src/NumberFormatterRegistry.js 100% <100%> (ø) ⬆️
...s/superset-ui-number-format/src/NumberFormatter.js 100% <100%> (ø) ⬆️
...er-format/src/factories/createD3NumberFormatter.js 100% <100%> (ø)
...mat/src/factories/createSiAtMostNDigitFormatter.js 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 431b7b2...9d6e19c. Read the comment docs.

@kristw kristw added the reviewable Ready for review label Nov 29, 2018
@kristw kristw added this to the v0.7.0 milestone Nov 29, 2018
Copy link
Contributor

@xtinec xtinec left a comment

Choose a reason for hiding this comment

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

LGTM.

});
describe('if it is invalid d3 formatString', () => {
it('The format function displays error message', () => {
const formatter = createD3NumberFormatter({ formatString: 'i-am-groot' });
Copy link
Contributor

Choose a reason for hiding this comment

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

😆

@kristw kristw merged commit 600426f into master Nov 29, 2018
@delete-merged-branch delete-merged-branch bot deleted the kristw--number-format branch November 29, 2018 22:09
@kristw kristw added #enhancement New feature or request and removed reviewable Ready for review labels Dec 6, 2018
kristw pushed a commit that referenced this pull request Apr 17, 2020
…#42)

Tap into the newly added `willUnmount` hook to clean up sticky tooltip that nvd3 adds to `body` directly. This removes such tooltips that were left behind in the explore view with the new query returns 'no data'.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
#enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants