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

AppSettings: Add FilePicker for notesPath #990

Closed
wants to merge 1 commit into from

Conversation

theatischbein
Copy link
Contributor

I created an drawft for #969 and added a FilePicker for the notesPath.

The problem I encounter is that the settings menu will be closed when clicking inside in the file picker modal.
I am unsure where it is triggered and how to prevent it. My guess it that is inherits the behaviour from NcAppNavigationSettings.
Can you help me out ?

Here a gif from my work:
nc_note_notespath

@juliushaertl
Copy link
Member

NcAppNavigationSettings has a excludeClickOutsideSelectors property that could work to handle this case by passing the selector of the file picker modal.

https://nextcloud-vue-components.netlify.app/#/Components/App%20containers/NcAppNavigation?id=ncappnavigationsettings

@juliushaertl juliushaertl added the enhancement New feature or request label Mar 22, 2023
@juliushaertl juliushaertl marked this pull request as ready for review March 22, 2023 19:47
@juliushaertl juliushaertl marked this pull request as draft March 22, 2023 19:48
@theatischbein
Copy link
Contributor Author

Thanks for that hint!
Do you have any experience using that property ? I am a little bit stucked right now.

If I am not mistaken it expects a string or array of CSS selectors. I determined that the main div from the FilePicker modal is oc-dialog.

<div class="oc-dialog" tabindex="-1" role="dialog" style="display: inline-block; position: fixed; width: 600px; height: 500px;">
    <h2 class="oc-dialog-title">Select folder to link to</h2>
    <a class="oc-dialog-close" tabindex="0"></a>    
....
	<div class="filelist-container">
...
</div><div class="oc-dialog-buttonrow onebutton aside"><button class="primary">Choose</button></div></div>

So I did:

<template>
	<NcAppNavigationSettings :title="t('notes', 'Notes settings')" :class="{ loading: saving }" :excludeClickOutsideSelectors="['.oc-dialog']">
		<div class="settings-block">
...

In difference to the deprecated excludeClickOutsideClasses I thought I needed to add to whole selector
https://github.com/nextcloud/nextcloud-vue/blob/4821f4a81492ca96068dec9a2c036c00644a3751/src/mixins/clickOutsideOptions/index.js#L47-L59

It does compile, but has no effect.
Does the exclution also not work for the children ?

@juliushaertl
Copy link
Member

I haven't used that yet. The only occurrence I could find was the deprecated property in the calendar app: https://github.com/nextcloud/calendar/blob/main/src/components/AppNavigation/Settings.vue#L23

@theatischbein
Copy link
Contributor Author

theatischbein commented Mar 22, 2023

Is it possible that we don't use nextcloud-vue 7.8.2 but some earlier version ?
excludedQuerySelectors is defined since 7.8.2, before only excludeClickOutsideClasses existed.
https://github.com/nextcloud/nextcloud-vue/blob/v7.8.1/src/mixins/excludeClickOutsideClasses/index.js

As you can see in my gif I am using excludeClickOutsideClasses and it partially works. I can click on the modal itself but as soon as I click on a folder the menu closes..
I guess it has something to do with the loading of the directory content.
Using excludedQuerySelectors it closes even on the outer modal divs.

ezgif com-video-to-gif

@juliushaertl
Copy link
Member

Right we are still on 7.3 there, let me check why dependabot is not suggesting an update there

https://github.com/nextcloud/notes/blob/master/package.json#L22

@juliushaertl
Copy link
Member

#999 needs a bit more of testing but I'll try to get to that later today.

@juliushaertl
Copy link
Member

Automation was faster but seems to work fine with the upgrade. Feel free to rebase your branch and try if that solves it.

@theatischbein
Copy link
Contributor Author

theatischbein commented Mar 23, 2023

Strangely with nextcloud/vue 7.8.2 none of the exclusion properties works..

In 7.3 I am pretty sure it is related to the the click event on the table row tr of the file picker. https://github.com/nextcloud/server/blob/55ba9601f2ef5b14524693a77c59cb7f7174270c/core/src/OC/dialogs.js#L452-L456
But only happening to the mouse click, because if I move with keyboard enter step into a folder with Enter the menu doesn't close..

But I can't explain why nothing works out right now.
Good things:

  • I fixed the mime type and it only shows folder right now
  • the filepicker starts at the last save location now

I need to experiment even more here to fix that bug (it feels like one)

@theatischbein
Copy link
Contributor Author

theatischbein commented Mar 23, 2023

I had second thoughts about this.
Since I call this.onChangeSettingsReload() after updating this.settings the menu will close during the reload.

So I guess this is also a design decision.
All other options do not have a Save button. I see why the input field had one, but now that this triggers the modal it doesn't have to be a form and need to be submitted manually.

I think I want to kind of ask if this worths the effort..

Edit: In the current version changing the notes path is the only option closing the settings menu due to reload. Which is needed to reload the new files if I am correct.

@juliushaertl
Copy link
Member

I only recently took over maintenance of the app so I cannot really tell much about previous design decisions, but would agree with you that closing the settings is not really a problem here then. Also nextcloud apps in general are more often moving away from the small settings area and towards a dedicated settings dialog.

The reload might also be only needed for historical reasons, but that might be a different story.

Overall I'd be totally fine with getting this in without keeping the settings opened. :)

@theatischbein
Copy link
Contributor Author

I did add a rather dirty workaround. Dirty because I temper with the style settings and because it is visual.
https://github.com/JonnyTischbein/notes/blob/10c90d4ccac4e15c11d1eac28ea93339941382d8/src/components/AppSettings.vue#L55-L67

I'm also fine with removing the workaround.

You can see my result here.
ezgif com-video-to-gif

The point it is weird is, if you abort the file select via the upper right X, than you have to click twice for closing the settings.

@theatischbein
Copy link
Contributor Author

theatischbein commented Mar 23, 2023

Also nextcloud apps in general are more often moving away from the small settings area and towards a dedicated settings dialog.

I could also try in an additional PR to move the app settings into personial setting page /settings/user

Or did you mean a modal window for settings like in the files app ?

for NCAppNavigationSettings excludeClickOutsideSelectors

Signed-off-by: Jonathan Pagel <jonny_tischbein@systemli.org>
theatischbein added a commit to theatischbein/notes that referenced this pull request Apr 15, 2023
Signed-off-by: Jonathan Pagel <jonny_tischbein@systemli.org>
@theatischbein
Copy link
Contributor Author

close in favour of #1003

theatischbein added a commit to theatischbein/notes that referenced this pull request Apr 15, 2023
Signed-off-by: Jonathan Pagel <jonny_tischbein@systemli.org>
theatischbein added a commit to theatischbein/notes that referenced this pull request Apr 15, 2023
Signed-off-by: Jonathan Pagel <jonny_tischbein@systemli.org>
theatischbein added a commit to theatischbein/notes that referenced this pull request Apr 26, 2023
Signed-off-by: Jonathan Pagel <jonny_tischbein@systemli.org>
theatischbein added a commit to theatischbein/notes that referenced this pull request Apr 26, 2023
Signed-off-by: Jonathan Pagel <jonny_tischbein@systemli.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants