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

Feature/788/toolbar #790

Open
wants to merge 62 commits into
base: main
Choose a base branch
from
Open

Feature/788/toolbar #790

wants to merge 62 commits into from

Conversation

newhinton
Copy link
Contributor

fixes #788

@newhinton newhinton changed the title Feature/788/toolbar [WIP] Feature/788/toolbar Dec 6, 2021
@newhinton newhinton changed the title [WIP] Feature/788/toolbar Feature/788/toolbar Dec 8, 2021
@newhinton
Copy link
Contributor Author

newhinton commented Dec 8, 2021

I have a working prototype now. It contains two important parts:

  1. The actual toolbar in the editor
  2. A new Sidemenu-Component

There are todo's for both parts:
Toolbar:

  • Decide what should be in the toolbar
  • Decide what should not be in the toolbar, but in the sidemenu
  • mobile styles

Sidemenu:

  • Naming
  • Interfacing from subcomponents. Currently they have to go through the note
  • Technical Review
  • properly set the toolbar up in mounted()

I have moved the three-dot menu to it's own component. This way we can add items to it dynamically, so that we can have different sets for the preview and the editor. While it works, i am not quite happy with how i implemented it. What i would like to do is to make this component "global", so that each subcomponent can manage its own entries instead of "abusing" the note-component.

Also i still need to add icons

@korelstar
Copy link
Member

Sorry, I don't get what you are trying to do with the three-dot menu. The code is not working (JavaScript errors and linting errors (please check make lint)). Could you provide a screenshot or sketch that shows how it should look like?

@newhinton
Copy link
Contributor Author

Ahh yes, it is currently not working, but i know why and will fix it.

Actually, there are no visible changes to the three-dot menu. Basically i want to extract the currently fixed size menu from the note.vue component, to it's own component. This allows other components, such as the editor to add its own entries. As an example: If we add both local image selection, and image uploading in the attachment-PR, both need buttons to trigger each action. However, some items should not be in the "primary" toolbar, but in an overflow menu.

Beeing in it's own subcomponent, we can archieve this. I am open to other suggestions though

@newhinton
Copy link
Contributor Author

newhinton commented Feb 19, 2022

@korelstar can you try again? It should hopefully "work" now, albeit there are still a lot of lint and js errors. I will fix them as soon as i know that this implementation of a sidemenu is actually a good idea. Also no icons as of yet.

Uploading works, though we may need to adjust the api to return the proper filename. I know why it fails, and i will open a seperate pr for it so that we can fast track and discuss it seperately.

@korelstar
Copy link
Member

Could you please explain in words what you are trying to do with that TheSidemenu component? I still don't get it. Do we really need this? It looks like this makes the whole thing very complicated. Can't we just leave the three-dot menu as it is and add all formatting options to the new toolbar?

Comment on lines 278 to 312
// the first "undo" is the editor beeing populated. Do not undo that!
if(this.mde.codemirror.historySize().undo>1){
this.mde.codemirror.undo()
}
Copy link
Member

@korelstar korelstar Feb 27, 2022

Choose a reason for hiding this comment

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

Suggested change
// the first "undo" is the editor beeing populated. Do not undo that!
if(this.mde.codemirror.historySize().undo>1){
this.mde.codemirror.undo()
}
if (this.mde.codemirror.historySize().undo > 0) {
this.mde.codemirror.undo()
}

This special case (undo>1) is not needed anymore, since #825 fixes the original issue! Thanks for pointing me on this!

@newhinton
Copy link
Contributor Author

Could you please explain in words what you are trying to do with that TheSidemenu component? I still don't get it. Do we really need this? It looks like this makes the whole thing very complicated. Can't we just leave the three-dot menu as it is and add all formatting options to the new toolbar?

The idea is to allow the editor to move items there. My reasoning is that if we put everything that is possible in the toolbar, we get quite a lot of icons that are not really necessary to be there. Basically an overflow menu. We also cant just put them, because the entries would be useless in the preview. Thats why i tried to make this menu dynamic.

Another idea i had was to add a secondary overflow menu to the toolbar, but then we would have two threedot menu's which i tried to avoid. I'm open to suggestions though!

@korelstar
Copy link
Member

To be honest, I don't think it's a good idea to put formatting actions into the existing three-dot menu. Which items do you want to put there exactly? Maybe we can group them into other "sub" menus, like the image actions in #823 .

@newhinton
Copy link
Contributor Author

You are right, i also was thinking about ditching that idea. I want to try to use the action-buttons but i have not gotten around to rebase this pr, but i will probably ditch the variable menu

@korelstar korelstar added the feature: EasyMDE Realted to the integrated EasyMDE editor label Mar 20, 2022
@newhinton
Copy link
Contributor Author

@korelstar I have removed the sidemenu and switched to Actions. (I have not yet rebased this pr)

However, the Nextcloud Vue Style Guide is not quite clear on how i would force all the actions to be shown, currently i only have the threedot menu. Do you know how i would force X elements to be shown outside the popover?

@korelstar
Copy link
Member

Could you please provide a screenshot? That would make it easier to understand the problem.

@newhinton
Copy link
Contributor Author

Sure!

grafik

As you can see, all the actions are stuck behind the group. Do you know if it is possible to only group them if there is no space? (or force x elements out?) The documentation is not really clear about it or i overlooked it.

@korelstar
Copy link
Member

As you can see, all the actions are stuck behind the group. Do you know if it is possible to only group them if there is no space? (or force x elements out?) The documentation is not really clear about it or i overlooked it.

I'm not sure if this is possible with nextcloud-vue. You may have to add multiple actions components. However, the Text app has realized this feature. Did you looked in the code from there?

@newhinton
Copy link
Contributor Author

I haven't found anything as of yet, i will have to dig deeper.
But generally: Is this the way to go? What do you think of the general idea, do i need to change anything?

@korelstar
Copy link
Member

I haven't found anything as of yet, i will have to dig deeper.

I think here is what you need: https://github.com/nextcloud/text/blob/master/src/components/MenuBar.vue

But generally: Is this the way to go? What do you think of the general idea, do i need to change anything?

For me, it looks good. But I would like to hear also some feedback from @nextcloud/notes (especially @stefan-niedermann ) and @nextcloud/designers (especially @jancborchardt ). What do you think?

One thing should also be improved: currently clicking a button adds more markdown even if the text is already formatted the same intended way. In this case, the existing formatting should be removed instead.

@korelstar
Copy link
Member

@newhinton Just found and read this: nextcloud/server#32060 . So, we will need to switch to vue-material-design-icons for this PR.

@newhinton
Copy link
Contributor Author

i glanced over that already, but yeah you are right.

We need to check if that is going to be compatible with nc22-24, otherwise we may need to provide two versions. But it's still WIP, so i will concentrate on getting the toolbar working in the way i'd like, and do that switch later

@stefan-niedermann
Copy link
Member

Looks good design wise. Maybe rename Monospace to the less technical term Code? I would also strip the "Insert" from the checkbox, just call it "Checkbox" like for bold, title etc.

@newhinton
Copy link
Contributor Author

@korelstar Your link to the menubar is what i want to achieve. Though, as far as i see it, i probably have to copy a lot of code or rewrite large parts of that menubar to make it work here.

I think it would be better if we could make the Actions component behave the same way in the nextcloud-vue component. This way others can use it aswell and we dont have to maintain it in this app.

If not, i will probably have to build my own system for singular ActionButton so that we get reusable code.

@korelstar
Copy link
Member

@korelstar Your link to the menubar is what i want to achieve. Though, as far as i see it, i probably have to copy a lot of code or rewrite large parts of that menubar to make it work here.

I think it would be better if we could make the Actions component behave the same way in the nextcloud-vue component. This way others can use it aswell and we dont have to maintain it in this app.

I agree that it is a good approach to move that code into a generic nextcloud-vue component. But I'm not sure if this should be in the Actions component or if it's better to create a new component (e.g. ActionsBar) - personally, I prefer the last. This should be discussed in the nextcloud-vue repository. Contributions are very welcome over there.

@newhinton
Copy link
Contributor Author

nextcloud-libraries/nextcloud-vue/issues/2655

I opened such an issue, though i dont think i will have time to develop such a component.

@skjnldsv
Copy link
Member

@newhinton Just found and read this: nextcloud/server#32060 . So, we will need to switch to vue-material-design-icons for this PR.

It's also much easier :)

@jancborchardt
Copy link
Member

For easier design review, before/after screenshots would be nice. :) Thanks!

@korelstar korelstar mentioned this pull request May 1, 2022
@newhinton
Copy link
Contributor Author

newhinton commented May 12, 2022

@korelstar I have removed the ActionButtons for now. I guess it will take a while to create an ActionGroup component in the vue repository, until then i switched back to normal buttons.

i also added a small convenience function where enter inserts an additional checkbox if the current line is a checkbox. (the same for dashes, eg. bulletlists.) It behaves the same way the android app does.

So, from a functionality standpoint this PR is done. It may need additional work, especially with tooltips for the buttons, and i think we could generally update the css a bit, but i think besides that i am done with the toolbar.

Before After
grafik grafik

@korelstar korelstar added this to the 4.5.0 milestone May 29, 2022
@korelstar korelstar modified the milestones: 4.5.0, 4.6.0 Aug 13, 2022
@max-nextcloud
Copy link

nextcloud-libraries/nextcloud-vue#3060 might come in handy even though it's not a full menubar yet as it does not change the number of displayed items responsively.

@korelstar korelstar modified the milestones: 4.6.0, 4.7.0 Oct 1, 2022
@juliusknorr juliusknorr modified the milestones: 4.7.0, 4.8.0 Mar 21, 2023
@juliusknorr
Copy link
Member

@newhinton Just wanted to check back if this is something you would still be interested to drive forward for the old editor, now that the text app integration with rich formatting and a toolbar is there?

@newhinton
Copy link
Contributor Author

@juliushaertl Generally yes, but i am very busy at the moment so progress will be slow. Though it will probably not too much work to update this, i will take a look on the weekens.

@juliusknorr juliusknorr added the enhancement New feature or request label Apr 26, 2023
@ant0nwax
Copy link

Hi all, I am a user of Nextcloud who left Google Keep.
I am just curious on the following and a simple yes or no is enough for me if you have not more time to answer, i can respect that. Uploading of Images does not work for me.
I answer myself for my setup:

I. Do image attachments in notes md files display in the for:

A. Nextcloud Files App Notes folder in Browser

  1. Mobile view? YES
  2. Desktop view? YES

B. Nextcloud Notes App in Browser

  1. Mobile view? NO
  2. Desktop view? NO

C. Nextcloud Android App? NO

II. Can new images be attached/uploaded to a notes md file for:

A. Nextcloud Files App Notes folder in Browser

  1. Mobile view? NO
  2. Desktop view? NO

B. Nextcloud Notes App in Browser

  1. Mobile view? NO
  2. Desktop view? NO

C. Nextcloud Android App? NO

So as you see in my environment (i could share details)
only cases I. A. 1) and I. A. 2) work

Does someone of you have other cases working? I ask because if that is the only working method yet I do not need to debug ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request feature: EasyMDE Realted to the integrated EasyMDE editor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add toolbar to native markdown editor