intechstudio / profile-cloud

0 stars 0 forks source link

🚩PR: Reworked description field and added Markdown and Images paste support #76

Closed elsoazemelet closed 3 months ago

elsoazemelet commented 3 months ago

Closes #74 Closes https://github.com/intechstudio/profile-cloud/issues/73

Image paste usage: Copy image data to clipbard then paste it into the description field

github-actions[bot] commented 3 months ago

Visit the preview URL for this PR (updated for commit ae48510):

https://profile-cloud-dev--pr76-feat-markdown-m0vos40r.web.app

(expires Mon, 17 Jun 2024 19:11:36 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: 2a004f867edf1347070dd9beedb18755187a6d4e

SukuWc commented 3 months ago

Markdown rendering looks to be appropriate, however some fixes are needed

  1. Please preserve original UX:
    • [x] double clicking enables editing if you have editing rights on the profile!
    • [x] click outside quites editing mode and renders the description as markdown
    • [x] no separate preview and edit tabs are needed
  2. Bugs:
    • If I select preview tab and now navigate to other profiles that are not owned by me the description field does not update
    • If I select edit tab and now navigate to other profiles that are not owned by me the description field becomes editable
elsoazemelet commented 3 months ago

SUKU comment from merged feat/Image branch PR:

Works as expected however it is seems unsafe to store arbitrary HTML in database and then just rendering it in someone else's session (XSS). Checked the store JSON file and it hase HTML elements and inline data image. Even if we escape HTML tags in the front-end we won't be able to safely store it in DB because back-end data sanitization will be implemented. After that the image tags will not ever render again. :(

Suggestion: only use markdown image tag to store the image! This is already supported by the chosen markdown renderer!

example:

# Example with Inline Image

This is an example of an image (red circle) embedded directly in the Markdown file using base64 encoding.

![Inline Image](data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAUAAAAFCAYAAACNbyblAAAAHElEQVQI12P4//8/w38GIAXDIBKE0DHxgljNBAAO9TXL0Y4OHwAAAABJRU5ErkJggg==)

You can see the image above.
elsoazemelet commented 3 months ago

Above mentioned suggestions are implemented in the latest commit!

narayb commented 3 months ago

Headings don't end with newline

Hitting Enter or Shift-Enter after a heading will make the heading formatting apply to all text below that. Expected behavior should be that it only applies to that line of text.

kép

Formatting behaves unexpectedly

When pressing the buttons Ctrl+B for bold or Ctrl+I for Italics, text changes accordingly, but the operators don't appear in the text.

kép

In markdown italics text should look like this *italics* when looking at the source. Same thing applies for bold and the bug probably affects all kinds of formatting as well, like strikethrough, underline etc.

In my opinion, hotkeys should format the text properly by inserting these special characters into the source code.

Not all features are supported

Seemingly lists or numbered lists are unsupported in this version of markdown.

And some of them breaks rendering completely as well.

kép

kkerti commented 3 months ago

What @narayb said above. And...

I think we can live without the shortcuts on initial release. But the basic markdown features should work.

Observation on the lists:

  1. Lists more or less work, when they are the first elements in the description field
  2. They appear to be working for a split second when they are not the first element Jun-06-2024 17-56-06

A stretch goal here would be a way to minify the image base64 code in the description field after pasting, but probably that's for the future.

elsoazemelet commented 3 months ago

Fixed before mentioned issues in the latest PR. Supperted features are mentioned by @kkerti