-
Notifications
You must be signed in to change notification settings - Fork 97
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
Add QR-Code for Share Links #2162
Conversation
8aa9ca3
to
125b678
Compare
@Himmelxd thanks for this PR :) I added some comments on the code |
ec92802
to
72ebf01
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some more comments and in my opinion improvements. @susnux Please also have a look at it 🙂
4936912
to
acc8f51
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So except those two comments (that slipped through before), everything looks fine from my side. Could you please squash all commits into one?
9d3afde
to
e71359c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine for me now :) @susnux please have another look
Thank you for your review and great comments :D |
9794111
to
41ab25d
Compare
d5ad0b1
to
8a4a227
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some small comments again... Please also do another rebase to squash all commits into one.
5137239
to
2888875
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the changes!
But I still have some changes 🙈
I meant to abstract the whole dialog as a component, so also move the NcDialog
to the QRDialog
component.
Some minor stuff like making it reactive (currently in your version the uri is only built on create).
PS: My code suggestions are only to show what I meant, they are not tested and need some refactoring ;) |
ddcb904
to
dd1351a
Compare
b57a36d
to
87b7b30
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copyright/license information is missing in QRDialog.vue
87b7b30
to
a76bf27
Compare
@Himmelxd to fix the remaining Lint/Prettier errors, you should run Depending on your editor, there are also some extensions that allow formatting single files directly. |
d207842
to
e65c830
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything's fine from my side :)
@susnux can we merge this now?
e65c830
to
a5eb28d
Compare
Signed-off-by: Felix Beichler <felix@beichlers.de> resolve first comments Signed-off-by: Felix Beichler <felix@beichlers.de> move modal into tab vue Signed-off-by: Felix Beichler <felix@beichlers.de> remove spaces (again) Signed-off-by: Felix Beichler <felix@beichlers.de> refactor qr-variables to single object on this. Signed-off-by: Felix Beichler <felix@beichlers.de> resolve comments Signed-off-by: Felix Beichler <felix@beichlers.de> remove url label Signed-off-by: Felix Beichler <felix@beichlers.de> resolve comments Signed-off-by: Felix Beichler <felix@beichlers.de> parameterize alt text Signed-off-by: Felix Beichler <felix@beichlers.de> fix order of iconqr import Signed-off-by: Felix Beichler <felix@beichlers.de> change nc modal to dialog Signed-off-by: Felix Beichler <felix@beichlers.de> resolve Share {formTitle} Signed-off-by: Felix Beichler <felix@beichlers.de> separate qr dialog to component Signed-off-by: Felix Beichler <felix@beichlers.de> resolve comments Signed-off-by: Felix Beichler <felix@beichlers.de> Move dialog into component Signed-off-by: Felix Beichler <felix@beichlers.de> fix package-lock Signed-off-by: Felix Beichler <felix@beichlers.de> add copyright/license section fix lint Signed-off-by: Felix Beichler <felix@beichlers.de> fix package.json
a5eb28d
to
fa6ecdb
Compare
@Himmelxd Thank you very much, awesome work 🚀 Hope to see more contributions in the future! |
Closes #1606
at least supposed to