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

Markdown for description #381

Merged
merged 6 commits into from
Jan 30, 2021
Merged

Markdown for description #381

merged 6 commits into from
Jan 30, 2021

Conversation

TheMBeat
Copy link
Contributor

No description provided.

@christianlupus christianlupus linked an issue Nov 10, 2020 that may be closed by this pull request
@sam-19
Copy link
Contributor

sam-19 commented Nov 10, 2020

Vue Showdown looks promising at a glance. NC Text (being the main official NC app that utilizes markdown, I imagine) uses markdown-it, but my impression is that they have not completely settled on a rendering library yet. So treading carefully and enabling only basic markdown syntax would probably be the best way to go for now, to avoid causing confusion with different sets of features in different apps.

@TheMBeat
Copy link
Contributor Author

What do you mean by enabling only the basic Markdown syntax? We can talk about the config in the main.js.
Currently there is no markdown editor available in the recipe editor, because the styling did not fit to the other input fields. But in the PR is one that is not activated.

@sam-19
Copy link
Contributor

sam-19 commented Nov 10, 2020

I mean that the 'github' flavor set for the editor supports at least two features that I believe NC Text does not: tables and emojis. Enabling features that are not available in other NC apps could cause confusion when, for example, copying and pasting content from one app to another. Just saying we should be mindful of this.

@TheMBeat
Copy link
Contributor Author

That makes sense. I will work on the settings again.

@sam-19: A bit offtopic, but could you take a look at my repo in the NewRecipeScraper branch to see if it would be possible to add this?

@seyfeb
Copy link
Collaborator

seyfeb commented Nov 10, 2020

I agree that we should clearly define the set of Markdown commands. Maybe we can consider using MultiMarkdown, which is also rather widespread?

@sam-19
Copy link
Contributor

sam-19 commented Nov 10, 2020

@sam-19: A bit offtopic, but could you take a look at my repo in the NewRecipeScraper branch to see if it would be possible to add this?

Wow, that's quite a bit to look at! There are some things that I'm not familiar with, but using a javascript parser for recipes is not something I would have a problem with (and I wouldn't mind being able to import recipes from my favorite recipe site allrecipes.org!). @christianlupus is really the guy to talk to though, since he's more familiar with the current parser. Maybe you should open an issue or PR for discussion, so this one won't get derailed?

@TheMBeat
Copy link
Contributor Author

@sam-19: A bit offtopic, but could you take a look at my repo in the NewRecipeScraper branch to see if it would be possible to add this?

Wow, that's quite a bit to look at! There are some things that I'm not familiar with, but using a javascript parser for recipes is not something I would have a problem with (and I wouldn't mind being able to import recipes from my favorite recipe site allrecipes.org!). @christianlupus is really the guy to talk to though, since he's more familiar with the current parser. Maybe you should open an issue or PR for discussion, so this one won't get derailed?

The problem is that it doesn't work properly because there are CORS problems with the fetch. I was hoping you could help.

@TheMBeat
Copy link
Contributor Author

With Vue-Showdown most options like table or emoji are disabled by default. For Style we could use Vanilla or Original instead of GitHub. With these settings we should be close to the Nextcloud Text app or am I wrong?

@christianlupus
Copy link
Collaborator

I would not worry too much about the set of Markdown commands available. It is merely a question of documentation. No user will open an issue stating that a certain feature has to be removed.
The bigger question is to ask which flavor to use to be maximally compatible with other (NC) apps. Just my 50ct.

I requested formal revieew of @sam-19 to check for correct integration. The question of the flavor might of course be an open discussion, as I think.

@codecov
Copy link

codecov bot commented Jan 3, 2021

Codecov Report

Merging #381 (378f95c) into master (30f2e93) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##             master    #381   +/-   ##
========================================
  Coverage      1.02%   1.02%           
  Complexity      434     434           
========================================
  Files            13      13           
  Lines          1366    1366           
========================================
  Hits             14      14           
  Misses         1352    1352           
Flag Coverage Δ Complexity Δ
integration 0.00% <ø> (ø) 0.00 <ø> (ø)
unittests 1.02% <ø> (ø) 0.00 <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

src/main.js Outdated Show resolved Hide resolved
@seyfeb
Copy link
Collaborator

seyfeb commented Jan 5, 2021

Maybe as a general comment: I don’t know the libraries, (vue) showndown seems to be quite active, v-markdown-editor seems less active (not that many commits, last publish 6 months ago) and also the size is quite big.

Assuming they don’t introduce any security holes, we could just keep it the way it is done here. This would be the quickest way. In the long term we could also replace v-markdown-editor by a custom component that also uses showndown to reduce the dependencies and app size.

If @TheMBeat doesn’t have the time to add the CSS necessary to enable the controls in the editor, atm, we could just merge this for now to enable Markdown rendering.

@TheMBeat
Copy link
Contributor Author

TheMBeat commented Jan 5, 2021

Hey @seyfeb my time is very limited. Also, I'm not very strong in CSS and had tried to use the toolbar on the first commit and failed, so it's disabled.
I like your idea with the custom component based on Vue Showdown, unfortunately I won't be able to do it anytime soon.

INTL\mauclair and others added 5 commits January 6, 2021 07:41
Signed-off-by: thembeat <thembeat@gmx.de>
Signed-off-by: Christian Wolf <github@christianwolf.email>
Signed-off-by: Marcel Auclair <thembeat@gmx.de>
Update URL

Update Text

Signed-off-by: thembeat <thembeat@gmx.de>
Signed-off-by: Christian Wolf <github@christianwolf.email>
Signed-off-by: Marcel Auclair <thembeat@gmx.de>
Signed-off-by: Christian Wolf <github@christianwolf.email>
Signed-off-by: Marcel Auclair <thembeat@gmx.de>
Signed-off-by: Marcel Auclair <thembeat@gmx.de>
Signed-off-by: Marcel Auclair <thembeat@gmx.de>
@christianlupus christianlupus mentioned this pull request Jan 29, 2021
@seyfeb seyfeb merged commit b42b21e into nextcloud:master Jan 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Markdown Support
4 participants