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.

Sign up for GitHub

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

Jump to bottom

Add full page markdown editor widget #3621

Merged
merged 18 commits into from
Oct 22, 2024
Merged

Add full page markdown editor widget #3621

merged 18 commits into from
Oct 22, 2024

Conversation

koopmant
Copy link
Contributor

@koopmant koopmant commented Oct 14, 2024 •
edited
Loading

This adds a Markdown editor with side-by-side editor and preview panels.

Height of the panels is linked. Size of the panels can be adjusted by resizing the textarea using the handle.
image

See also:

https://github.com/DIAGNijmegen/rse-roadmap/issues/351

closes #3609

@koopmant
Rename MarkdownEditorWidget to MarkdownEditorInlineWidget
510d3e5 
Refactor to clarify new type (full page widget, to be added)
@koopmant
Create side by side version of markdown editor widget
220bd2e 
Note: this breaks the inline widget, the linked resizing javascript is applied to both versions of the widget. In case of the inline widget, we don't want that.
@koopmant
Set textarea to monospace
72a3bae
@koopmant
Pre-commit fixes
5b6c244
@koopmant
Split extra javascript to new file
631f99f 
To ensure it's only applied to "full page" editor widgets
@koopmant
Add "Preview" text and adjust formatting
6aaf5a2
@koopmant
Pre-commit fixes
27cab7e 
Pre-commit hook was not installed yet, so had to run it manually.
@koopmant
Add docs
5c04f63
@koopmant koopmant linked an issue Oct 14, 2024 that may be closed by this pull request
Add full page markdown editor widget #3609
Closed
@bramvanginneken
Copy link
Member

bramvanginneken commented Oct 14, 2024 via email

@koopmant
Copy link
Contributor Author

Who is ‘Thomas’?

Hi Bram, I started at the UvA on October 1st in the qurAI group (under Clarisa Sanchez). I'm also the newest member of the grand-challenge RSE group. Nice to meet you!

@bramvanginneken
Copy link
Member

bramvanginneken commented Oct 15, 2024 via email

@jmsmkn jmsmkn changed the title 3609 add full page markdown editor widget Add full page markdown editor widget Oct 15, 2024
HarmvZ
HarmvZ requested changes Oct 15, 2024
View reviewed changes
Copy link
Member

@HarmvZ HarmvZ left a comment

Choose a reason for hiding this comment

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

Just a partial review of only the js file here. I have a few small comments.

Also, did you try what happens when there are multiple markdown widgets on a single page? Looking at your code I suspect it will execute the js for each widget. So that means it will find all markdown widgets on the page and apply the resize observer to each of them. So multiple widgets on one page will duplicate all the resize observers for each widget. You can avoid this by replacing the .getElementsByClassName("markdownx"); call with a specific call that only finds the current widget using its unique id. You can pass that unique id to javascript using the json_script jinja filter.

@bramvanginneken

This comment was marked as off-topic.

Sign in to view
@koopmant
Cleanup javascript
dd8c20b 
avoid use of jquery, fix variable name case and use for-loop instead of map function
@koopmant
Remove extra css file
23a8de9 
Replace with bootstrap class.
Abandoned setting initial height of editor; will pick it up later during implementation.
@pkcakeout
Copy link
Contributor

pkcakeout commented Oct 21, 2024 •
edited
Loading

@koopmant - that last edit you did to Bram's message is rather confusing to me... it now looks like Bram is talking to himself... I think that was not the intention... may I suggest another edit? (Please undo my edit in case you wanted to achieve something else)

Oh god... I now see what has happened: GitHub butchers editing an email reply. Good to know. And nvm: Note-to-self: never edit an emailed-in reply.

@koopmant
Copy link
Contributor Author

Just a partial review of only the js file here. I have a few small comments.

Also, did you try what happens when there are multiple markdown widgets on a single page? Looking at your code I suspect it will execute the js for each widget. So that means it will find all markdown widgets on the page and apply the resize observer to each of them. So multiple widgets on one page will duplicate all the resize observers for each widget. You can avoid this by replacing the .getElementsByClassName("markdownx"); call with a specific call that only finds the current widget using its unique id. You can pass that unique id to javascript using the json_script jinja filter.

Before we dive too deep into this, I'm probably removing this script to keep the solution simple. Nonetheless:

I tested that. When there are multiple widgets, the javascript is still only included once. I suspect because Django handles this for me when it loads the form.

@koopmant
Copy link
Contributor Author

@koopmant - that last edit you did to Bram's message is rather confusing to me... it now looks like Bram is talking to himself... I think that was not the intention... may I suggest another edit? (Please undo my edit in case you wanted to achieve something else)

Oh god... I now see what has happened: GitHub butchers editing an email reply. Good to know. And nvm: Note-to-self: never edit an emailed-in reply.

Hi Paul, it was even more confusing than that. I wanted to test out the email reply function. So I replied by e-mailing back. However, that message was posted as if Bram had sent my email. So I then edited it to make it more clear the message came from me.

@koopmant
Set initial height of full page editor
b5c05e1
@koopmant
Avoid applying js to inline widgets
20863f2 
In case both full page and inline widgets are included on the same page
@koopmant
Avoid inline style
30069cc
@jmsmkn
Copy link
Member

jmsmkn commented Oct 22, 2024

That looks like a bug in GitHub, you should not be able to post comments as another user. I can't find anything on it after a quick search so please report it to them.

The pre-commit checks are failing. @koopmant please install pre-commit and run pre-commit install in you checkout of this repo, that way the checks will run locally before every commit and that check will always pass. Sometimes manual interaction is required.

@pkcakeout
Copy link
Contributor

pkcakeout commented Oct 22, 2024 •
edited
Loading

That looks like a bug in GitHub, you should not be able to post comments as another user. I can't find anything on it after a quick search so please report it to them.

@koopmant - I would be happy to report the issue, but you encountered the original bug where Bram's message was exchanged for yours. You would need to add the details about that. I think that this is the severe issue of the two. The half-baked non-markdown editing is probably just a missing feature in general. So could you take that on? I can independently report the horrible experience when trying to edit an email response.

@koopmant
Copy link
Contributor Author

That looks like a bug in GitHub, you should not be able to post comments as another user. I can't find anything on it after a quick search so please report it to them.
@pkcakeout @jmsmkn I'll report it.

@koopmant
Copy link
Contributor Author

The pre-commit checks are failing. @koopmant please install pre-commit and run pre-commit install in you checkout of this repo, that way the checks will run locally before every commit and that check will always pass. Sometimes manual interaction is required.

@jmsmkn I had to reinstall my system (I'll explain later...) and forgot to install pre-commit (again). Then ran into the same issue as @ammar257ammar. (biomejs/pre-commit#11 biomejs/pre-commit#13). I added a temporary fix in this PR. See .pre-commit.config.yaml. Can I leave this in? I can also remove it at the end, but I need it for now, otherwise pre-commit won't run.
PS Is there a reason pre-commit is not mentioned in pyproject.toml?

@jmsmkn
Copy link
Member

jmsmkn commented Oct 22, 2024

Yes that's fine. You could also make another PR with the fix to have it on main.

It's not in pyproject.toml as I've not thought of it as a development dependency (e.g. what do you need to run the tests). Looking at Django they also omit it. I'm not sure how it behaves when you have multiple installs as it installs environments globally, so like poetry it's something you install globally and use.

@comic comic deleted a comment from bramvanginneken Oct 22, 2024
@bramvanginneken
Copy link
Member

bramvanginneken commented Oct 22, 2024 via email

@koopmant
Copy link
Contributor Author

It's not in pyproject.toml as I've not thought of it as a development dependency (e.g. what do you need to run the tests). Looking at Django they also omit it. I'm not sure how it behaves when you have multiple installs as it installs environments globally, so like poetry it's something you install globally and use.

Ok, yes that makes sense.

@jmsmkn jmsmkn merged commit d777e2b into main Oct 22, 2024
8 checks passed
@jmsmkn jmsmkn deleted the 3609_add_full_page_markdown_editor_widget branch October 22, 2024 13:56
@koopmant koopmant mentioned this pull request Oct 23, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@HarmvZ HarmvZ HarmvZ requested changes

@jmsmkn jmsmkn jmsmkn approved these changes

@amickan amickan Awaiting requested review from amickan amickan is a code owner

Assignees

@koopmant koopmant

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add full page markdown editor widget
5 participants
@koopmant @bramvanginneken @pkcakeout @jmsmkn @HarmvZ