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
@kristw kristw added this to Needs review in Current PRs Status 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.

😆

Current PRs Status automation moved this from Needs review to Reviewer approved Nov 29, 2018
@kristw kristw merged commit 600426f into master Nov 29, 2018
Current PRs Status automation moved this from Reviewer approved to Done 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'.
zhaoyongjie pushed a commit to zhaoyongjie/superset-ui that referenced this pull request Sep 23, 2021
zhaoyongjie pushed a commit to zhaoyongjie/superset-ui that referenced this pull request Sep 24, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
#enhancement New feature or request
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants