search

short-d / short

URL shortening service written in Go and React
https://short-d.com
MIT License
870 stars 148 forks source link

Added missing port info in the frontend link. #967

Closed MrClan closed 4 years ago

MrClan commented 4 years ago

Current Behavior ( Optional for new feature )

Port info is missing in the generated frontend link due to which the link is not correct when the port is other than port 80.

Description

Screenshots

New Behavior

Added port info in the generated frontend link.

Description

Screenshots

codecov[bot] commented 4 years ago

Codecov Report

Merging #967 into master will not change coverage. The diff coverage is 0.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #967   +/-   ##
=======================================
  Coverage   53.05%   53.05%           
=======================================
  Files         142      142           
  Lines        3498     3498           
  Branches      166      166           
=======================================
  Hits         1856     1856           
  Misses       1577     1577           
  Partials       65       65
Flag Coverage Δ
#golang 72.00% <ø> (ø)
#typescript 22.84% <0.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
frontend/src/service/Url.service.ts 0.00% <0.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 6c721b0...aa2aeb8. Read the comment docs.

rohithbalaji123 commented 4 years ago

Won't this change make the created URL look something like in the image? I am not sure if we want to show the port there. But yeah, this issue needs to be addressed.

image
MrClan commented 4 years ago

Yes, it will show the port in the link but it should be okay since it is the correct url.

rohithbalaji123 commented 4 years ago

@MrClan , actually, window.location.port is empty in production site. I guess we should use, host instead of hostname https://stackoverflow.com/a/11379802/6533004

magicoder10 commented 4 years ago

I don't like having port visible on the UI. To users, it introduces unnecessary complexities. Can we use environmental variable to only show the port for local development?

MrClan commented 4 years ago

image

The change has been verified on the testing environment and the screenshot shows that the port is not displayed in the url.