Skip to content
This repository was archived by the owner on Jan 16, 2022. It is now read-only.

fix: 76 download button hidden for localhost#101

Merged
juanpicado merged 5 commits intoverdaccio:4.x-masterfrom
griffithtp:fix/76_download-button-hidden-for-localhost
Jul 14, 2019
Merged

fix: 76 download button hidden for localhost#101
juanpicado merged 5 commits intoverdaccio:4.x-masterfrom
griffithtp:fix/76_download-button-hidden-for-localhost

Conversation

@griffithtp
Copy link
Copy Markdown
Member

@griffithtp griffithtp commented Jul 13, 2019

Type: bugfix

The following has been addressed in the PR:

  • There is a related issue? #76
  • Unit or Functional tests are included in the PR - YES

Description:
As described in the attached issue 76, download tarball files were not showing for localhost as it is not TLD.
Also, in this PR we refactor url.ts and history.ts by removing unused functions and files being called once only.

Resolves #76

@codecov-io
Copy link
Copy Markdown

codecov-io commented Jul 13, 2019

Codecov Report

Merging #101 into 4.x-master will increase coverage by 0.08%.
The diff coverage is 100%.

Impacted file tree graph

@@              Coverage Diff               @@
##           4.x-master     #101      +/-   ##
==============================================
+ Coverage       70.27%   70.36%   +0.08%     
==============================================
  Files              89       88       -1     
  Lines             858      857       -1     
  Branches          156      155       -1     
==============================================
  Hits              603      603              
+ Misses            243      242       -1     
  Partials           12       12
Impacted Files Coverage Δ
src/utils/url.ts 100% <ø> (+20%) ⬆️
src/router.tsx 82.35% <100%> (+2.35%) ⬆️

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 46ae0d2...ecc4521. Read the comment docs.

DanielRuf
DanielRuf previously approved these changes Jul 13, 2019
juanpicado
juanpicado previously approved these changes Jul 13, 2019
@griffithtp griffithtp dismissed stale reviews from juanpicado and DanielRuf via 6222b64 July 13, 2019 14:28
@griffithtp
Copy link
Copy Markdown
Member Author

Added unit test for isURL() and isEmail() however I am not entirely sure there is a purpose having getBaseNamePath() and getRootPath() in the file.
Shall we remove them if it's used only once, the other not used?

@griffithtp griffithtp force-pushed the fix/76_download-button-hidden-for-localhost branch from 6222b64 to 795544a Compare July 13, 2019 14:46
@juanpicado
Copy link
Copy Markdown
Member

The method getBaseNamePath is definitely used, but about getRootPath I'd like to investigate what happened with the place was this being used before remove it.

@griffithtp
Copy link
Copy Markdown
Member Author

The method getBaseNamePath is definitely used, but about getRootPath I'd like to investigate what happened with the place was this being used before remove it.

getBaseNamePath() only used in history.ts and unless we can anticipate future use it might simpler without it and consolidate test on history.test.ts.

const history = createBrowserHistory({
  basename: window.__VERDACCIO_BASENAME_UI_OPTIONS && window.__VERDACCIO_BASENAME_UI_OPTIONS.url_prefix,
});

Anyway this perhaps is irrelevant in this PR 😄

@juanpicado
Copy link
Copy Markdown
Member

ahh, let's do it now, make sense, :-)

@juanpicado juanpicado requested a review from DanielRuf July 14, 2019 12:06
@juanpicado
Copy link
Copy Markdown
Member

Thanks @griffithtp 🚀

@juanpicado juanpicado merged commit cad5ac9 into verdaccio:4.x-master Jul 14, 2019
@juanpicado juanpicado added enhancement New feature or request and removed PR: needs review labels Jul 14, 2019
@griffithtp griffithtp deleted the fix/76_download-button-hidden-for-localhost branch July 14, 2019 16:08
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.

4 participants