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

implement in-app-wiki #862

Merged
merged 33 commits into from
Oct 18, 2022
Merged

implement in-app-wiki #862

merged 33 commits into from
Oct 18, 2022

Conversation

newhinton
Copy link
Contributor

@newhinton newhinton commented May 26, 2022

Signed-off-by: Felix Nüsse felix.nuesse@t-online.de
closes #287 and closes #793

Todo:

  • Add close button to wikipage
  • Add translations everywhere
  • Fix offset in Navigationbar
  • Add tips
  • Make tables look better
  • Embedd appstore icons
  • Fix styling for Apps
  • fix all console errors
  • add link to help to welcome page (maybe even replace it?)
  • create sample-note from wiki and welcome page
Welcomescreen Help Help
grafik grafik grafik

Discussion:

  • What shortcuts to add
  • What markdown codes to add
  • What tipps to add

@newhinton
Copy link
Contributor Author

newhinton commented May 27, 2022

@korelstar I would like to not reference the appstore badges (and app-icons maybe) via url. I would like to embedd them as assets. However, i could not find out how nextcloud stores static assets, do you know that? One clunky way would be to embedd them base64 encoded in a helper class, but that seems unclean

Edit: found out!

@newhinton
Copy link
Contributor Author

grafik

@stefan-niedermann How would you like to be attributed for your app? Is this okay?

@newhinton
Copy link
Contributor Author

I think we could easily put this with all the other outstanding features into a Version 5.0.0. It's a nice round number, and there was quite a bit of work done to justify it 👍

@korelstar
Copy link
Member

Hey @newhinton : Thanks for your work on this! The screenshots are looking very well so far. Since you are still actively working on this PR, I wait with a final review. But it's going in a good direction!

But I have one request: could you please set this (and other not yet finished) PRs to "draft"? It would be very helpful to have a direct insight if a PR is ready for review or not.

Regarding the version name: I'm using the milestone planing. The plan was to release version 4.4.0 several weeks ago. It should have contained all the stuff that change the public API (attachments, custom file extension) -- but the last thing is still left, since I've not found the time :-( . After that, the plan was to release 4.5.0 with all the UI stuff (three-column layout, help-screen, toolbar etc.). Version 5.0.0 was planned with the switch to the Nextcloud Text editor, but that is currently halted due to the open issues regarding the Text app.

@korelstar korelstar added this to the 4.5.0 milestone May 29, 2022
@newhinton
Copy link
Contributor Author

Yes i will convert everything to drafts soon.

However, this is not so drafty-anymore, it is mostly done and we need to discuss content.

Regarding 5.0.0: Since we more or less implement everything text does (except concurrent editing), do we need that issue at all? It does not seem that the issues will be fixed anytime soon, if at all, and since we have a very well working setup with the new additions, i think we can do without it until something major changes.

@newhinton newhinton changed the title implement in-app-wiki Draft: implement in-app-wiki May 29, 2022
@newhinton newhinton marked this pull request as draft May 29, 2022 20:26
@newhinton newhinton changed the title Draft: implement in-app-wiki implement in-app-wiki May 29, 2022
@newhinton newhinton marked this pull request as ready for review August 6, 2022 13:14
@korelstar korelstar modified the milestones: 4.5.0, 4.6.0 Aug 13, 2022
@korelstar
Copy link
Member

@newhinton Is there anything to be done here? Could you please rebase the branch when this is ready to review?

@newhinton
Copy link
Contributor Author

Yes and no :D

I think the technical side is done. There was the question wether or not to replace the initial welcome-page with a popup, but this can be done later.

However, we should check two things:
First, if all app-maintainer are okay with beeing mentioned on that page
And second, if we have all keyboard-shortcuts and tips in the help that we want.

But besides that, i think its good to go. (When i will rebase this, i'll also check if there are still console errors)

@korelstar
Copy link
Member

First, if all app-maintainer are okay with beeing mentioned on that page

Any veto @stefan-niedermann @phedlund ?

And second, if we have all keyboard-shortcuts and tips in the help that we want.

I'll do a deeper review.

package-lock.json Outdated Show resolved Hide resolved
src/App.vue Outdated Show resolved Hide resolved
src/App.vue Outdated Show resolved Hide resolved
src/components/AppHelp.vue Outdated Show resolved Hide resolved
src/components/AppHelp.vue Outdated Show resolved Hide resolved
src/components/AppHelp.vue Outdated Show resolved Hide resolved
{ shortcut: t('notes', 'CTRL+Alt+L'), action: t('notes', 'kes the current line a list element with a number') },
{ shortcut: t('notes', 'SHIFT+CTRL+H'), action: t('notes', 'toggleHeadingBigger') },
{ shortcut: t('notes', 'F9'), action: t('notes', 'toggleSideBySide') },
{ shortcut: t('notes', 'F11'), action: t('notes', 'Make the note fullscreen') },
Copy link
Member

Choose a reason for hiding this comment

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

Hmm. This is not the same like the fullscreen action in the three-dots menu. Hence, remove this entry?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But it is? In the end the tree dot menu calls "requestFullscreen()" on the dom, which acts the exact same way as F11.

Copy link
Member

Choose a reason for hiding this comment

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

At least not in my Firefox: pressing F11 goes into fullscreen but all the Nextcloud frame and app navigation is still visible. Using the fullscreen option shows the note only without any distracting frame.

src/components/AppHelp.vue Show resolved Hide resolved
Comment on lines 73 to 83
createNote('')
.then(note => {
const query = { new: getDefaultSampleNote() }
this.$router.push({
name: 'note',
params: { noteId: note.id.toString() },
query,
})
})
.catch(() => {
})
Copy link
Member

Choose a reason for hiding this comment

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

I don't like this approach, because it generates a very long URI. Instead, you should add a second parameter to createNote that takes the content and directly sends it to the server in the same request that creates the new note.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Besides the query, this is how the sidebar is doing it. (If i remember correctly, that's where i found the code in the first place. I just extracted the query so that i could add content more cleanly.

Should we change that in this PR?

Copy link
Member

Choose a reason for hiding this comment

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

Sidebar? What do you mean?

But yes, please do not put a whole note in a query parameter. :-)

Comment on lines 222 to 227

const newContent = this.$route.query.new
if (this.isNewNote && (newContent !== '' && newContent != null)) {
this.note.content = newContent
this.onManualSave()
}
Copy link
Member

Choose a reason for hiding this comment

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

I don't like this approach, because it generates a very long URI. Instead, you should add a second parameter to createNote that takes the content and directly sends it to the server in the same request that creates the new note.

@stefan-niedermann
Copy link
Member

How would you like to be attributed for your app? Is this okay?

That's fine, thanks 👍

Any veto @stefan-niedermann?

Nope 🙂

@korelstar korelstar modified the milestones: 4.6.0, 4.7.0 Oct 1, 2022
@newhinton
Copy link
Contributor Author

@korelstar I have implemented a lot of the changes you mentioned.

However, do we have any tips we want to add? Otherwise i need to remove that section from the help.

@newhinton
Copy link
Contributor Author

newhinton commented Oct 3, 2022

Oh damn. I broke something badly, i somewhere have the three column layout merged in there.

Fixed it.

Copy link
Member

@korelstar korelstar left a comment

Choose a reason for hiding this comment

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

@korelstar I have implemented a lot of the changes you mentioned.
However, do we have any tips we want to add? Otherwise i need to remove that section from the help.

Thanks so far! I think it can be removed, for now. Would you mind doing a rebase (due to #928)?

Comment on lines 5 to 7
<button class="button-icon-add icon-add" @click="onNewNote">
{{ t('notes', 'Create a sample note with markdown') }}
</button>
Copy link
Member

Choose a reason for hiding this comment

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

Please use NcButton from @nextcloud/vue instead

Comment on lines 16 to 18
<button class="button-icon-add icon-add" @click="onNewNote">
{{ t('notes', 'Create a sample note with markdown') }}
</button>
Copy link
Member

Choose a reason for hiding this comment

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

Please use NcButton from @nextcloud/vue instead

@newhinton
Copy link
Contributor Author

newhinton commented Oct 4, 2022

@korelstar So i did rebase, but it did not want to push. For now i have not force pushed my code, but created a second branch.

You can check it here: https://github.com/nextcloud/notes/tree/feature/noid/help2

If it works for you, i can force push later.

There is one bug i encountered: Closing or Opening the sidebar toggles the help. I will have to check that

@newhinton newhinton mentioned this pull request Oct 4, 2022
Signed-off-by: Felix Nüsse <felix.nuesse@t-online.de>
Signed-off-by: Felix Nüsse <felix.nuesse@t-online.de>
Signed-off-by: Felix Nüsse <felix.nuesse@t-online.de>
newhinton and others added 18 commits October 16, 2022 12:24
Signed-off-by: Felix Nüsse <felix.nuesse@t-online.de>
Signed-off-by: Felix Nüsse <felix.nuesse@t-online.de>
Signed-off-by: Felix Nüsse <felix.nuesse@t-online.de>
Signed-off-by: Felix Nüsse <felix.nuesse@t-online.de>
Signed-off-by: Felix Nüsse <felix.nuesse@t-online.de>
Signed-off-by: Felix Nüsse <felix.nuesse@t-online.de>
Update Text

Signed-off-by: Felix Nüsse <felix.nuesse@t-online.de>
Signed-off-by: Felix Nüsse <felix.nuesse@t-online.de>
fix padding for help

Signed-off-by: Felix Nüsse <felix.nuesse@t-online.de>
Signed-off-by: Felix Nüsse <felix.nuesse@t-online.de>
Signed-off-by: Felix Nüsse <felix.nuesse@t-online.de>
@korelstar
Copy link
Member

Hey @newhinton ! I did a rebase of feature/noid/help2 to current master and fixed all the issues that I've criticized. I've pushed the new stuff to feature/noid/help (this PR). Please have a look from your side on this. I will do a last review after that.

Signed-off-by: korelstar <korelstar@users.noreply.github.com>
@newhinton
Copy link
Contributor Author

@korelstar It looks good to me! Thanks for helping out

@korelstar korelstar merged commit c9d8af5 into master Oct 18, 2022
@korelstar korelstar deleted the feature/noid/help branch October 18, 2022 06:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add formatting help Overview of supported Markdown syntax
3 participants