-
Notifications
You must be signed in to change notification settings - Fork 96
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
base: main
Are you sure you want to change the base?
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.
f38aa2c
to
5137239
Compare
Signed-off-by: Felix Beichler <[email protected]> resolve first comments Signed-off-by: Felix Beichler <[email protected]> move modal into tab vue Signed-off-by: Felix Beichler <[email protected]> remove spaces (again) Signed-off-by: Felix Beichler <[email protected]> refactor qr-variables to single object on this. Signed-off-by: Felix Beichler <[email protected]> resolve comments Signed-off-by: Felix Beichler <[email protected]> remove url label Signed-off-by: Felix Beichler <[email protected]> resolve comments Signed-off-by: Felix Beichler <[email protected]> parameterize alt text Signed-off-by: Felix Beichler <[email protected]> fix order of iconqr import Signed-off-by: Felix Beichler <[email protected]> change nc modal to dialog Signed-off-by: Felix Beichler <[email protected]> resolve Share {formTitle} Signed-off-by: Felix Beichler <[email protected]> separate qr dialog to component Signed-off-by: Felix Beichler <[email protected]> resolve comments Signed-off-by: Felix Beichler <[email protected]>
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).
<div class="qrDialog__content"> | ||
<h2>{{ title }}</h2> | ||
<div> | ||
<img :src="uri" | ||
:title="text" | ||
:alt="t('forms', 'QR code representation of {text}', { text: text })"> | ||
</div> | ||
</div> |
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.
<div class="qrDialog__content"> | |
<h2>{{ title }}</h2> | |
<div> | |
<img :src="uri" | |
:title="text" | |
:alt="t('forms', 'QR code representation of {text}', { text: text })"> | |
</div> | |
</div> | |
<NcDialog class="qr-dialog" | |
close-on-click-outside | |
:name="title" | |
:open="open" | |
@close="$emit('close')> | |
<div class="qr-dialog__content"> | |
<img :src="uri" | |
:title="text" | |
:alt="t('forms', 'QR code representation of {text}', { text: text })"> | |
</div> | |
</NcDialog> |
The idea is to wrap the dialog not only the content, especially to use the NcDialog name
instead of custom heading.
created() { | ||
this.generateQr() | ||
}, |
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.
created() { | |
this.generateQr() | |
}, | |
watch { | |
text: { | |
immediate: true, | |
handler() { | |
this.generateQr() | |
}, | |
}, | |
}, |
Allow to be reactive to prop changes.
.qrDialog__content { | ||
margin-bottom: 50px; | ||
text-align: center; | ||
} |
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.
.qrDialog__content { | |
margin-bottom: 50px; | |
text-align: center; | |
} | |
.qr-dialog__content { | |
display: flex; | |
justify-content: space-around; | |
width: 100%; | |
} |
@@ -60,6 +60,12 @@ | |||
</template> | |||
{{ t('forms', 'Copy to clipboard') }} | |||
</NcActionLink> | |||
<NcActionButton @click="openQrDialog(getPublicShareLink(share))"> |
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.
<NcActionButton @click="openQrDialog(getPublicShareLink(share))"> | |
<NcActionButton @click="openQrDialog(share)"> |
Better to handle the logic in the method than in the template.
<NcDialog :open.sync="qrDialog" close-on-click-outside="true" @close="qrDialog=false"> | ||
<QRDialog :title="t('forms', 'Share {formTitle}', { formTitle: form.title })" | ||
:text="qrDialog" /> | ||
</NcDialog> |
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.
<NcDialog :open.sync="qrDialog" close-on-click-outside="true" @close="qrDialog=false"> | |
<QRDialog :title="t('forms', 'Share {formTitle}', { formTitle: form.title })" | |
:text="qrDialog" /> | |
</NcDialog> | |
<QRDialog :open="qrDialogText !== null" | |
:title="t('forms', 'Share {formTitle}', { formTitle: form.title })" | |
:text="qrDialogText" | |
@close="qrDialogText = null" /> |
Abstract the dialog and move it to the component instead.
@@ -232,6 +249,7 @@ export default { | |||
return { | |||
isLoading: false, | |||
appConfig: loadState(appName, 'appConfig'), | |||
qrDialog: false, |
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.
qrDialog: false, | |
qrDialogText: null, |
Mixing boolean and string here is not really obvious I think, so instead make it string by default with null
to explicitly mark no value.
async openQrDialog(qrText) { | ||
this.qrDialog = qrText | ||
}, |
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.
async openQrDialog(qrText) { | |
this.qrDialog = qrText | |
}, | |
openQrDialog(share) { | |
this.qrDialogText = getPublicShareLink(share) | |
}, |
see above (also this is not async)
PS: My code suggestions are only to show what I meant, they are not tested and need some refactoring ;) |
Closes #1606
at least supposed to