Skip to content

Conversation

JordanDC2
Copy link
Contributor

@JordanDC2 JordanDC2 commented Feb 17, 2022

The last circular dependency fix merely changed where the circular dependency occurred, as Footer/Summar.tsx imports from Footer/Cell.tsx and vice versa. So I Isolated the SummaryContext constant and the type it uses to its own file to break that circular import.

Verified the fix using madge https://www.npmjs.com/package/madge

@vercel
Copy link

vercel bot commented Feb 17, 2022

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.

🔍 Inspect: https://vercel.com/react-component/table/FNDKBRtyyv8qKmySXLfVrmQmhQ5f
✅ Preview: https://table-git-fork-jordandc2-fix-summary-con-a83496-react-component.vercel.app

@codecov
Copy link

codecov bot commented Feb 17, 2022

Codecov Report

Merging #754 (1f2a069) into master (9d4b3e4) will not change coverage.
The diff coverage is 100.00%.

❗ Current head 1f2a069 differs from pull request most recent head bfd8039. Consider uploading reports for the commit bfd8039 to get more accurate results

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #754   +/-   ##
=======================================
  Coverage   99.38%   99.38%           
=======================================
  Files          34       35    +1     
  Lines         976      976           
  Branches      281      281           
=======================================
  Hits          970      970           
  Misses          6        6           
Impacted Files Coverage Δ
src/Footer/Cell.tsx 100.00% <ø> (ø)
src/Footer/Summary.tsx 100.00% <ø> (ø)
src/Footer/index.tsx 100.00% <ø> (ø)
src/Footer/SummaryContext.tsx 100.00% <100.00%> (ø)

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 9d4b3e4...bfd8039. Read the comment docs.

@afc163
Copy link
Member

afc163 commented Feb 18, 2022

Could you add a test case?

@JordanDC2
Copy link
Contributor Author

Could you add a test case?

I did not add anything new here I just moved the constant to its own file, and the coverage report already shows 100% coverage for the files i've changed. In theory the new file should already be covered by the existing tests. Am I missing something?

@afc163 afc163 merged commit 312b95b into react-component:master Feb 19, 2022
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.

2 participants