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

[full-ci] Improvements to text/md editor #6667

Merged
merged 8 commits into from Apr 7, 2022
Merged

Conversation

diocas
Copy link
Contributor

@diocas diocas commented Mar 25, 2022

I'm trying to push upstream some improvements that we have in our own text-editor so that we can drop it. This PR is to discuss the direction and to have some pointers on how to implement things (sorry, I'm not well versed in Vue, and even less in composition api).

What this PR adds:

  • Prevents the browser from unloading the page if there's still unsaved changes
  • Allows ctrl/cmd+s for saving
  • Displays proper error messages when saving
  • Asks the user if he wants to save changes when exiting (either by clicking X or go back/forward in history, which might be by mistake)

Still in early stages, I'm also trying to make the app configurable, because we might want to remove the preview pane, since we also want to disable the markdown support (check #6661 )

@update-docs
Copy link

update-docs bot commented Mar 25, 2022

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

@diocas diocas marked this pull request as draft March 25, 2022 10:00
@diocas diocas requested a review from kulmann March 25, 2022 10:01
Copy link
Member

@dschmidt dschmidt left a comment

Choose a reason for hiding this comment

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

Nice start, I have some hints for improvements, but I like where this is going :)

packages/web-app-markdown-editor/src/App.vue Outdated Show resolved Hide resolved
packages/web-app-markdown-editor/src/App.vue Outdated Show resolved Hide resolved
packages/web-app-markdown-editor/src/App.vue Outdated Show resolved Hide resolved
@diocas
Copy link
Contributor Author

diocas commented Mar 31, 2022

@dschmidt thanks for your feedback, it helped me implementing what was missing as well.
I've done what you suggested and finalized what was pending:

  • Support for read-only files (the user cannot edit the file, only see). I've opened this bug as well: owncloud-design-system#2053
  • Design polish
  • Configurable preview pane (it's enabled for md files and disabled for others by default)
  • No auth in route, for compatibility with public links
  • More file extensions (it would be great if we could open no extension files..)

I've also renamed it from markdow-editor to simple-editor. I have the impression this naming is clearer for normal users, but if you prefer I can revert that commit.

What is missing is an option to disable md creation, since we would prefer to do it with codimd.
We could also group extensions in the new menu.. But ok, that's a new ticket on its own.

I might need another iteration with the tests.

@diocas diocas marked this pull request as ready for review March 31, 2022 14:58
@diocas diocas changed the title WIP Improvements to text/md editor Improvements to text/md editor Mar 31, 2022
Copy link
Member

@dschmidt dschmidt left a comment

Choose a reason for hiding this comment

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

Much better! I like!

...fileExtensionConfig
})
}

Copy link
Member

Choose a reason for hiding this comment

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

Not a fan of this, maybe we can register editors for mimetypes in the long run?

As we probably don't have a mechanism for that currently, it's totally out of scope for this PR. Maybe you can open a ticket?

Copy link
Member

Choose a reason for hiding this comment

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

Actually, I built registering apps on mime types in #6514 but that one got stale...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 for that! I would like to be able to open files with no extension.

Copy link
Member

Choose a reason for hiding this comment

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

I hope you don't get too overexited with this. Turns out that mime type guessing from the backend isn't always what you'd expect. E.g. ogg video files still get announced with audio/ogg as mime type. But I continued the PR and it's close to being merged.

@@ -17,8 +19,8 @@ const fileExtensionConfig = {
}

const appInfo = {
name: 'MarkdownEditor',
id: 'markdown-editor',
name: 'SimpleEditor',
Copy link
Member

Choose a reason for hiding this comment

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

totally fine with the rename, it's scoped way better than markdown-editor. 👍

@ownclouders
Copy link
Contributor

ownclouders commented Apr 6, 2022

Results for oC10Files2 https://drone.owncloud.com/owncloud/web/24564/18/1

💥 The acceptance tests failed on retry. Please find the screenshots inside ...

webUIFilesActionMenu-fileFolderActionMenu_feature-L14.png

webUIFilesActionMenu-fileFolderActionMenu_feature-L14.png

@@ -78,7 +78,7 @@ config = {
"oC10Files1": [
"webUIFilesCopy",
"webUIFavorites",
"webUIMarkdownEditor",
"webUISimpleEditor",
Copy link
Contributor

Choose a reason for hiding this comment

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

@diocas that would need a rename of https://github.com/owncloud/web/tree/master/tests/acceptance/features/webUIMarkdownEditor in order to work :) since it's mapping feature files/folders

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ups

@kulmann kulmann changed the title Improvements to text/md editor [full-ci] Improvements to text/md editor Apr 6, 2022
@kulmann
Copy link
Member

kulmann commented Apr 6, 2022

@diocas could you search&replace various combinations of the word markdown editor? I found at least one more markdowneditor in an acceptance test feature file. That should probably be simpleeditor now in that case. But maybe there's more.
Also I added [full-ci] to the PR title so that upon your next push the CI won't stop on the first failed acceptance test pipeline but will push through with all pipelines. That should give us a clear picture of what's missing for merge.

Support for read-only files
Disable preview for non md files (configurable)
Data integrity checks (prevent exiting with unsaved changes)
Better error information to the user
Keyboard shortcut to save
Enable text editor for other extensions
@diocas
Copy link
Contributor Author

diocas commented Apr 7, 2022

I actually did find&replace, but it took me some time to realize that I filter some folders when I'm in dev mode... Should be fine now, but let's see how the tests go.

@diocas
Copy link
Contributor Author

diocas commented Apr 7, 2022

Btw, what about the disabling of md file creation?

@pascalwengerter
Copy link
Contributor

Btw, what about the disabling of md file creation?

What do you mean?

@kulmann
Copy link
Member

kulmann commented Apr 7, 2022

Btw, what about the disabling of md file creation?

Can you open an issue please?

Copy link
Member

@kulmann kulmann left a comment

Choose a reason for hiding this comment

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

Found a typo. Fixing that should finally turn the test suite green 🥳

…nMenu.feature

Co-authored-by: Benedikt Kulmann <benedikt@kulmann.biz>
@diocas
Copy link
Contributor Author

diocas commented Apr 7, 2022

@kulmann I had already created #6661, let me know if I need to rephrase it.

@sonarcloud
Copy link

sonarcloud bot commented Apr 7, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot E 1 Security Hotspot
Code Smell A 2 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants