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

Add basic text editor using textarea #107

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

MahmoudHamdy02
Copy link
Collaborator

@MahmoudHamdy02 MahmoudHamdy02 commented Sep 4, 2023

  • Fix context menu:
    Context menu breaks after closing the editor, either after 'save' or 'cancel'. After changing directory it works normally until the page is refreshed
  • Add tests

@jelly
Copy link
Member

jelly commented Sep 5, 2023

I am not sure if this is the right UX pattern as we don't have an example of an editor anywhere in Cockpit, but it might be worth it to add a beforeunload event so when a user accidently closes the tab they get a warning. @garrett

@garrett
Copy link
Member

garrett commented Sep 12, 2023

I am not sure if this is the right UX pattern as we don't have an example of an editor anywhere in Cockpit, but it might be worth it to add a beforeunload event so when a user accidently closes the tab they get a warning. @garrett

Yes, set this in an onChange event (so it will only block when the content has changed) and make sure it's unset when the modal is dismissed with an action (save, cancel).

In other words, if nothing has changed in the textarea (or if it was saved or discarded), then it shouldn't block closing the page. But if something was changed, then block the page.

@MahmoudHamdy02 MahmoudHamdy02 force-pushed the add-textarea-editor branch 3 times, most recently from 8559b25 to 81d4754 Compare September 13, 2023 23:06
src/app.jsx Outdated Show resolved Hide resolved
src/app.jsx Outdated Show resolved Hide resolved
Copy link
Collaborator

@KKoukiou KKoukiou left a comment

Choose a reason for hiding this comment

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

thanks - I like it much more.

src/FileEditor.jsx Show resolved Hide resolved
src/app.jsx Outdated Show resolved Hide resolved
@MahmoudHamdy02
Copy link
Collaborator Author

Using cockpit.file().replace() with empty text will delete the file. We can use truncate -s 0 until the bug is fixed in the bridge.

@garrett
Copy link
Member

garrett commented Sep 19, 2023

I tried this branch, but the directory and files area is blank for me. (It's fine in main.)

image

@jelly jelly marked this pull request as draft March 15, 2024 08:49
@garrett
Copy link
Member

garrett commented Apr 9, 2024

To make it clear what I was thinking about, I've included a design @ #63:

I remember some discussion about this somewhere, but can't find it.

There were issues with the PatternFly code editor widget. I don't know of all the details. But the most basic first pass would be something like this:

image

It uses a basic textarea (HTML, but probably PatternFly's wrapper to the HTML element). It is a modal so you don't lose your place in the file browsing and can edit one file after another. On the left is editing mode; on the right is view-only / read-only.

First pass could be read-only viewing. Second pass could be editing. Third pass could be some kind of code viewer enhancement. However, the goal should be quick and easy simple editing of plain text files only, not a full-blown IDE (or even a full text editor like vim or emacs), so stopping at pass 2 (editable text area), at least for a while, is fine.

@garrett
Copy link
Member

garrett commented Jun 6, 2024

We'd want to make the size as large as possible vertically; there should be a little bit of space around it, but most of it should be editing. Otherwise, it's going to be annoying to use. For example:

image

@mac2net
Copy link

mac2net commented Jun 6, 2024

I wouldn't over-engineer this.
It's better to ship something and get feedback.
People can still also use the 45drives one in the meantime.

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

Successfully merging this pull request may close these issues.

None yet

5 participants