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

fix(lib): default size for content loader viewbox #76

Merged
merged 1 commit into from
Jun 30, 2021

Conversation

nileshbhagwat
Copy link
Contributor

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[x] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

What is the current behavior?

Issue Number: #72

What is the new behavior?

Adding default value for "viewBox" input (previous default value of width and height)

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

@NetanelBasal
Copy link
Member

Thanks. But based on what you decide the default value?

@nileshbhagwat
Copy link
Contributor Author

In v6, input width and height is replaced with the input "viewBox". Default values taken from those previous input to keep it consistent

image

Any suggestion?

@NetanelBasal
Copy link
Member

Can you please check the original React library?

@nileshbhagwat
Copy link
Contributor Author

Hi @NetanelBasal,

React version has hardcoded viewbox 0 0 340 84 for custom content loader
https://github.com/danilowoz/react-content-loader/blob/master/src/web/presets/CodeStyle.tsx

Also, OOB loaders have default value for viewbox 0 0 476 124
https://github.com/ngneat/content-loader/blob/master/projects/ngneat/content-loader/src/lib/facebook-loader.component.ts

Can we consider adding default value for custom content loader as well instead 0 0 0 0 ?

FYI, we are using old version 4.0.0. this change would be helpful to avoid inconsistency with library upgrade.

@NetanelBasal
Copy link
Member

Sure you can create a PR, but mark it as breaking change in the commit, please.

@nileshbhagwat nileshbhagwat force-pushed the fix/issues/72 branch 2 times, most recently from 7119be2 to c4203de Compare June 30, 2021 13:24
BREAKING CHANGE: added default value to "viewBox" input
@nileshbhagwat
Copy link
Contributor Author

Done, commit updated with breaking change description

@NetanelBasal NetanelBasal merged commit a749240 into ngneat:master Jun 30, 2021
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