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

Added fillTheParentElement feature and added responsive sizes about issue #27 #52

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

mustafapsd
Copy link

Hi! I've used your package in my several projects, thank you! I added two features. It is shown in this video.

#27

Video

@mnahkies mnahkies self-requested a review February 12, 2022 11:27
@mustafapsd
Copy link
Author

Hi! Is everything okey? Should I change anything?

Copy link
Owner

@mnahkies mnahkies left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR @mustafapsd - I can understand the motivation behind it, but I'm not completely comfortable with the current implementation, and wonder if it would make more sense to be a wrapper component around <qr-code> that controlled the size property

Specifically the need to have refreshing properties gives me the most concern. I've left some comments going into more detail - let me know what you think

projects/ng-qrcode-demo/src/app/app.component.scss Outdated Show resolved Hide resolved
projects/ng-qrcode-demo/src/app/app.component.ts Outdated Show resolved Hide resolved
projects/ng-qrcode/src/lib/qr-code.directive.ts Outdated Show resolved Hide resolved
projects/ng-qrcode/src/lib/qr-code.component.ts Outdated Show resolved Hide resolved
@mustafapsd mustafapsd requested a review from mnahkies February 17, 2022 12:07
@mustafapsd
Copy link
Author

I changed the logic. Users can manage the size from css. To be honest, I've liked this way.

Video

@mustafapsd
Copy link
Author

@mnahkies hi, is there any progress?

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