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 button to files app that opens the current folder in the photos app #607

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

cnmicha
Copy link

@cnmicha cnmicha commented Dec 23, 2020

Closes #566

This implementation provides the following UI:
image

Please note the following things:

  • We are hooking into the files app using CSS selectors. I did not find an API from the files app that allows custom UI controls being inserted at this place.
  • Tooltips do not have the Nextcloud styling yet and are currently missing localization.

@skjnldsv
Copy link
Member

cc @jancborchardt

package.json Outdated Show resolved Hide resolved
}

function redirectToPhotosApp() {
window.location.href = buildPhotosUrl()
Copy link
Member

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

Copy link
Author

@cnmicha cnmicha Dec 24, 2020

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.

Copy link
Member

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/

Copy link
Author

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

Copy link
Author

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?

webpack.js Outdated Show resolved Hide resolved
Copy link
Member

@skjnldsv skjnldsv left a comment

Choose a reason for hiding this comment

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

See comments

@skjnldsv skjnldsv added 2. developing Work in progress enhancement New feature or request labels Dec 24, 2020
@skjnldsv skjnldsv modified the milestones: Nextcloud 21, Nextcloud 22 Dec 24, 2020
css/filesbutton.scss Outdated Show resolved Hide resolved
@cnmicha
Copy link
Author

cnmicha commented Dec 27, 2020

@skjnldsv Is there an easy way to edit l10n? I see you have both .js and .json files

@skjnldsv
Copy link
Member

@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

@cnmicha
Copy link
Author

cnmicha commented Dec 27, 2020

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:
photos

But the tooltips from the files app button beneath looks like the following:
files

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...
Or we just accept the difference in design.

@skjnldsv What do you think?

@cnmicha
Copy link
Author

cnmicha commented Dec 27, 2020

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

@skjnldsv
Copy link
Member

But the tooltips from the files app button beneath looks like the following:

It's fine! It's because Files isn't using the vue library yet. It will change later on :)

But I have no idea what causes the issue

If you click on the ci check results, you will see that compiled files are missing: "Uncommited changes in webpack build"
You can ignore until this pr is approved, we'll use the compile bot to add the compiled files to this pr. Can you rebase and squash into one commit please? :)

@cnmicha cnmicha marked this pull request as ready for review December 29, 2020 14:16
@aredey
Copy link

aredey commented Feb 24, 2021

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?
Many thanks, Akos

@cnmicha
Copy link
Author

cnmicha commented Feb 24, 2021

@aredey I will implement the change requested by @skjnldsv today, afterwards I think merging the pull is up to him.

@cnmicha cnmicha force-pushed the feature/upstream-566 branch 3 times, most recently from 9e7a057 to fc57635 Compare February 24, 2021 13:45
Copy link
Member

@skjnldsv skjnldsv left a comment

Choose a reason for hiding this comment

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

See comments :)

src/FilesButton.vue Outdated Show resolved Hide resolved
src/FilesButton.vue Outdated Show resolved Hide resolved
src/FilesButton.vue Outdated Show resolved Hide resolved
src/FilesButton.vue Outdated Show resolved Hide resolved
src/FilesButton.vue Outdated Show resolved Hide resolved
src/FilesButton.vue Outdated Show resolved Hide resolved
@Mikescops
Copy link
Member

@cnmicha nice PR, thanks :)

@cnmicha
Copy link
Author

cnmicha commented Apr 2, 2021

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/.
So I will wait until a beta version of NC 22 is released?

@Mikescops
Copy link
Member

@cnmicha just git clone the nextcloud/server and you'll have the latest dev version :)

@skjnldsv skjnldsv mentioned this pull request Oct 25, 2021
25 tasks
@skjnldsv skjnldsv modified the milestones: Nextcloud 23, Nextcloud 24 Oct 25, 2021
@blizzz blizzz modified the milestones: Nextcloud 24, Nextcloud 25 Apr 21, 2022
@blizzz blizzz modified the milestones: Nextcloud 25, Nextcloud 26 Sep 22, 2022
@blizzz blizzz modified the milestones: Nextcloud 26, Nextcloud 27 Mar 9, 2023
@skjnldsv skjnldsv removed this from the Nextcloud 27 milestone May 10, 2023
@skjnldsv skjnldsv marked this pull request as draft May 10, 2023 09:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2. developing Work in progress enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Open folder in Photos button
5 participants