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

Settings: Move Settings to NcAppSettingsDialog, NotePath FilePicker and merge AppHelp #1003

Merged
merged 3 commits into from
Apr 26, 2023

Conversation

theatischbein
Copy link
Contributor

@theatischbein theatischbein commented Mar 25, 2023

Closes #969

Like discussed in #990 I just the Settings Dialog instead of the Navigation Settings.

For the settings button I had to use a css workaround from the files app.

Personally I would like to merge this PR with the file picker from #990 as reference I can recommend to look at the photos app.

Screenshot from 2023-03-25 09-12-37

@juliusknorr
Copy link
Member

I think it makes sense to combine this directly with #990.

We could even follow the Talk app and merge the help sections about keyboard shortcuts also into the same dialog:

Screenshot 2023-04-13 at 23 31 01

@theatischbein
Copy link
Contributor Author

I'll merge these two PRs.

Looking at #967 I would keep everything from the help menu except the markdown hints because they are already in the richtext editor, wouldn't I ?

@theatischbein
Copy link
Contributor Author

theatischbein commented Apr 15, 2023

Fortunately there is no need anymore for my workaround when clicking inside the filePicke :)

I wasn't sure how you handle commits in this project. I did a seperate commit for every step, but I would also squash them if that fits your guideline!

Current state:

Screenshot from 2023-04-15 09-38-32
Screenshot from 2023-04-15 09-38-37
image
image

@theatischbein theatischbein changed the title Settings: Move Settings to NcAppSettingsDialog in favour of the NcAppNavigationSettings Settings: Move Settings to NcAppSettingsDialog and merge AppHelp Apr 15, 2023
@theatischbein
Copy link
Contributor Author

@juliushaertl Any update on this ?
Can I do anything ? Do you need more testing ?

@theatischbein theatischbein changed the title Settings: Move Settings to NcAppSettingsDialog and merge AppHelp Settings: Move Settings to NcAppSettingsDialog, NotePath FilePicker and merge AppHelp Apr 26, 2023
@juliusknorr juliusknorr self-requested a review April 26, 2023 13:33
@juliusknorr
Copy link
Member

Looks great from the screenshots. I'll review it later today. Sorry for the delay.

@juliusknorr
Copy link
Member

Regarding the commits, individual ones are fine if they are cleanly separated as yours are 👍

NcAppNavigationSettings

Signed-off-by: Jonathan Pagel <jonny_tischbein@systemli.org>
Signed-off-by: Jonathan Pagel <jonny_tischbein@systemli.org>
<AppSettings v-if="!loading.notes && error !== true" @reload="reloadNotes" />
<ul class="app-navigation-entry__settings">
<NcAppNavigationItem
:title="t('notes', 'Notes settings')"
Copy link
Member

Choose a reason for hiding this comment

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

Not blocking as we can follow up.

@nimishavijay What would be your take on the wording here? Should we maybe call this "Notes settings / help" since the content is a bit more than just settings? See screenshots from before for a full overview.

Copy link
Member

Choose a reason for hiding this comment

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

Hm, that's a good point. Looking at other apps, Talk and Calendar have keyboard shortcuts as part of their settings. So I think it is expected to have shortcuts there :) As a follow up, there could be the functionality of pressing the key ? to bring up the keyboard shortcuts section of the settings, but that is totally a future enhancement. For now I'd say the wording is fine as it is :)

Copy link
Member

@juliusknorr juliusknorr left a comment

Choose a reason for hiding this comment

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

Great work :) Looks great and works like a charm. Lets get that merged and we can follow up on the wording depending on further input from Nimisha.

@juliusknorr juliusknorr added enhancement New feature or request design Related to the design or user experience labels Apr 26, 2023
@juliusknorr juliusknorr added this to the 4.8.0 milestone Apr 26, 2023
@juliusknorr
Copy link
Member

@jonnytischbein Could you fix the remaining lint errors?

Signed-off-by: Jonathan Pagel <jonny_tischbein@systemli.org>
@theatischbein
Copy link
Contributor Author

Thank you so much! :)

I missed to run lint at the end again. Fixed it now

@juliusknorr juliusknorr merged commit 66a5e4d into nextcloud:master Apr 26, 2023
@juliusknorr juliusknorr mentioned this pull request May 12, 2023
22 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design Related to the design or user experience enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"Folder to save your notes" input would be nicer and safer as a folder picker dialog
3 participants