Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(content-loader): set baseUrl to location.pathname when initialized #42

Merged
merged 1 commit into from
Aug 18, 2019

Conversation

leo6104
Copy link
Contributor

@leo6104 leo6104 commented Aug 16, 2019

With this commit, we don't need to pass [baseUrl] everytime for Safari browser.

It will set baseUrl to window.location.pathname if it can access to window object.

@leo6104
Copy link
Contributor Author

leo6104 commented Aug 16, 2019

I read the issue (danilowoz/react-content-loader#93) and PR (#38) and understand all situations.

I know that it is not same way to support baseUrl parameter in react-content-loader implementation.
However, In Angular, <base href="/"> tag is default option in index.html so i think it would be better to always set to location.pathname. Please consider it :)

@NetanelBasal
Copy link
Member

And it will also work without the baseUrl?

@leo6104
Copy link
Contributor Author

leo6104 commented Aug 17, 2019

@NetanelBasal Yes. I apply this commit on our company services and it works without baseUrl.

3 condition i considered.

  1. If user set [baseUrl] input, it will not overwrite to location.pathname and use the input.
  2. If user not set [baseUrl], it will set baseUrl variable to location.pathname automatically.
  3. If it is server-side rendering, not set to window.location.pathname. (because window object is not defined in Node environment)

@NetanelBasal
Copy link
Member

Resolve the conflicts, please.

@leo6104 leo6104 changed the title fix(content-loader): set baseUrl to location.pathname when initialized feat(content-loader): set baseUrl to location.pathname when initialized Aug 18, 2019
@leo6104
Copy link
Contributor Author

leo6104 commented Aug 18, 2019

Resolved! :)

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