Skip to content
This repository was archived by the owner on May 6, 2025. It is now read-only.

Basic QR Functionality #126

Merged
merged 3 commits into from
Mar 20, 2019
Merged

Basic QR Functionality #126

merged 3 commits into from
Mar 20, 2019

Conversation

jackklika
Copy link
Contributor

@jackklika jackklika commented Mar 19, 2019

Close #56

I have no experience with Vue and very little with npm, but for my own instance wanted to have some basic QR code functionality on the share page.

In this small feature, whenever the Share view is displayed, it will show a QR code that links directly to the download.

I'm sure this isn't up to par with the rest of the code, but it should be a starting point for someone that knows Vue best practices.

Let me know what you think, and I'd appreciate it if you could point out any problems or lead me on a path to making this small feature better and more in line with the project's vision.

-jack

@eine
Copy link
Contributor

eine commented Mar 19, 2019

Hi @jackklika! Thanks for this PR. Overall, it LGTM, although I have not actually built it. Some minor issues:

@jackklika
Copy link
Contributor Author

  1. I'll check out the conflict and resolve it when I get a chance.
  2. Nothing strong, besides that it has more downloads on npm, has two collaborators vs one, and the documentation made it easier for me to integrate it into the code as a beginner to Vue. Also, what does "FTR" mean? I'm not familiar with the acronym.

@eine
Copy link
Contributor

eine commented Mar 19, 2019

  1. I'll check out the conflict and resolve it when I get a chance.

Great. Whenever you can.

  1. Nothing strong, besides that it has more downloads on npm, has two collaborators vs one, and the documentation made it easier for me to integrate it into the code as a beginner to Vue.

Good.

Also, what does "FTR" mean? I'm not familiar with the acronym.

For The Record. It is meant for anyone in the future checking the commit history and coming back to this issue; so that they have some brief explanation.

@jackklika
Copy link
Contributor Author

I tested this on my personal instance and it seems to work.

Still hazy on the npm situation, so let me know if I resolved the merge conflict incorrectly.

@eine eine merged commit 4e15b82 into filebrowser:master Mar 20, 2019
@eine
Copy link
Contributor

eine commented Mar 20, 2019

Thanks @jackklika!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants