-
-
Notifications
You must be signed in to change notification settings - Fork 62
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 button to files app that opens the current folder in the photos app #607
base: master
Are you sure you want to change the base?
Conversation
src/filesbutton.js
Outdated
} | ||
|
||
function redirectToPhotosApp() { | ||
window.location.href = buildPhotosUrl() |
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.
You can also directly use a link instead of a button. That would be more adequate regarding the html semantics
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.
I agree, but I don't see how this is possible.
In https://github.com/cnmicha/photos/blob/feature/upstream-566/src/FilesButton.vue#L50-L52, the buildPhotosUrl()
function seems to only work when the files app has done some internal logic. Before (and on vue init time), FileList.getCurrentDirectory()
is not available.
I did not find a corresponding event emitted by the files app we could register on.
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.
Use an ActionLink instead of an ActionButton
https://nextcloud-vue-components.netlify.com/
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.
@skjnldsv Done but the pipeline seems to be stuck (the checks don't report results). I already tried again by changing the commit id and force-pushing, but no luck
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.
@skjnldsv Any news on this?
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.
See comments
6ae636a
to
1d4545d
Compare
@skjnldsv Is there an easy way to edit l10n? I see you have both .js and .json files |
They are managed automatically from the transifex website |
To implement the tooltip, I switched to the ActionButton Vue component that is also used in other parts of the photos app. It looks like this: But the tooltips from the files app button beneath looks like the following: Please note that the tooltips have different font sizes and positions and that the photos button has a grey circular background. The files app is not using the ActionButton component, instead they seem to use jQuery tooltips... (see here and here) I think this does not look very appealing, but we would either a) need to change the files app to use the ActionButton component as well (which would be a lot of effort because they are not using Vue right now), b) add jQuery as a dependency to the photos app or c) create our own variant of the ActionButton component which would add more maintenance work... @skjnldsv What do you think? |
Looking at the diffs, it seems as if webpack compiles the files differently on my machine resulting in the "Node / node12.x (pull request)" checks failing. But I have no idea what causes the issue. I am using the default configuration. Here is the output of the compile command and git status: https://pastebin.com/yyH93f0c |
It's fine! It's because Files isn't using the vue library yet. It will change later on :)
If you click on the ci check results, you will see that compiled files are missing: "Uncommited changes in webpack build" |
7958afd
to
4415ca9
Compare
Hi All. Looking forward to this feature in the Photos application, when is it likely to be released for production (or potentially for 3rd party texting)? Will this view be exposed as default for non-logged-in users if the folder is publicly shared? |
9e7a057
to
fc57635
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.
See comments :)
@cnmicha nice PR, thanks :) |
It seems that I can't test the photos app (fork of the master branch) because it forces me to use NC 22, which is not released yet. I didn't find a prerelease available on https://download.nextcloud.com/server/prereleases/. |
@cnmicha just git clone the nextcloud/server and you'll have the latest dev version :) |
Closes #566
This implementation provides the following UI:
Please note the following things: