-
Notifications
You must be signed in to change notification settings - Fork 22
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
feat(article): Add MD support #1451
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for gno-dapp ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for teritori-dapp ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
12ef521
to
271f701
Compare
2bb197f
to
a8cab39
Compare
a8cab39
to
6408c7a
Compare
packages/screens/FeedNewArticle/components/ArticleContentEditor/utils.ts
Outdated
Show resolved
Hide resolved
In term of UI what do you think about show borders for the editor and the preview ? |
return ( | ||
<ScreenContainer | ||
forceNetworkFeatures={[forceNetworkFeature]} | ||
responsive | ||
mobileTitle="NEW ARTICLE" | ||
fullWidth | ||
headerChildren={<ScreenTitle>New Article</ScreenTitle>} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please keep ScreenTitle
to be consistent with the new title UI
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
9c4bf23
Thx for this new component btw ^^
control={control} | ||
rules={{ | ||
required: true, | ||
<TextInputCustom<NewArticleFormValues> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should avoid at maximum custom components, if it's not possible nevermind but we should trying to be consistent imo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't agree. I think it's better to have our custom versions of RN components, to centralize styles and logics.
(Especially with forms)
Just for this example, it's heavy to re-handle <Label/>
, onHover
, form control, all the feedbacks, in many components, it's better to factorize.
Maybe the term "custom" is not good ? How to name our Teritori components ?
But TextInputCustom
needs some refacto, it's a mess...
In term of behvior found that enough for a first version and we can iterate on add styles just after. |
it would also be nice (in another PR since it's not the scope) but to use the start of the article to make a preview subtitle and let it optional to write a custom preview, if you agree i will open a new issue |
If you talk about the white borders, it's the same behavior than on other inputs. I try to put this behavior on each input, I love this UI that highlights interactive elements on hover such as inputs or buttons.. In mobile format, I removed the white borders padding, because there is no hover. If to talk about a separation between preview and edition, why not adding one, yes :) Personnaly I prefer pure without separation, it's enough excplicit |
These are draft buttons, I'm looking for better buttons, an other style for these buttons, maybe with icons, something cute. |
i think we mean don't just show the border on hover and only when hover but always show a border, the border could become fully white on hover and be gray when not hover but for now when you don't hover the input, the input is mix with background making it impossible to know what is the border of the input |
I don't have final opinion on border, in fact you can do as you want, i think it can be discuss later because it could be nice to merge it asap 👍 |
windowWidth < RESPONSIVE_BREAKPOINT_S ? 0 : SOCIAl_CARD_BORDER_RADIUS; | ||
|
||
const metadata = zodTryParseJSON( | ||
ZodSocialFeedArticleMetadata, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we shouldn't reuse the old type for the new article kind IMO
so create a new schema ZodSocialFeedArticleMarkdownMetadata
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, but we didn't discuss about the new shape. I don't want to commit, then fix the review, then commit, then fix the review, etc. We need to status on that once
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay, can you duplicate the schema for now please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
packages/components/socialFeed/SocialCard/cards/SocialArticleMarkdownCard.tsx
Outdated
Show resolved
Hide resolved
@MikaelVallenet , it was an idea at start: Using the first image as "cover image", the first h1 as title and first text under h1 as subtitle. We don't have "cover image" anymore since the user can just add an image before the h1 with markdown. The "automatic title and subtitle" was not good with the old Article version. We should put the preview after the content writting. It needs another PR maybe |
Ok I see, letting the input borders always visible as other inputs. I would like to let it epurated, I have some arguments, but it's better to debate in vocal IMO. |
@MikaelVallenet I reused these boxes to harmoinze visually, we could change, etc 9c42f45#diff-33ab4e16a7693f5368d5ec3b64f8b8b182598fc5b91135abcc0c228bbeac6f69R1-R74 |
…using domVisitors prop
@@ -123,6 +123,8 @@ const gradient = (type: GradientType): LinearGradientProps => { | |||
return getMapPostTextGradient(PostCategory.Normal); | |||
case getMapPostTextGradientType(PostCategory.Article): | |||
return getMapPostTextGradient(PostCategory.Article); | |||
case getMapPostTextGradientType(PostCategory.ArticleMarkdown): | |||
return getMapPostTextGradient(PostCategory.Article); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this on purpose ?
Needs, todos, etc => https://hackmd.io/@9Oh5kimjS1SP8xUWU_-Ovg/HkcuBoN4yx
We use this library to generate HTML from Markdown syntax: https://github.com/markdown-it/markdown-it
We use this library to render the HTML into JSX: https://github.com/meliorence/react-native-render-html
MarkdownIt
Here, we initialize
MarkdownIt
with some parameters, and use the Articlemessage
to render HTML: https://github.com/TERITORI/teritori-dapp/blob/feat-feed-article-markdown/packages/screens/FeedNewArticle/components/ArticleContentEditor/ArticleContentEditor.tsx#L53We use the same
MarkdownIt
parameters at Article creation and Article consultation (WYSIWYG).RenderHtml
We pass custom styles to
RenderHtml
. Each style item correspond to an html tag. So, there are a lot of customisable styles. We could customise the most used only: https://github.com/TERITORI/teritori-dapp/blob/feat-feed-article-markdown/packages/screens/FeedNewArticle/components/ArticleContentEditor/ArticleContentEditor.tsx#L169Here are the currently customized styles: https://github.com/TERITORI/teritori-dapp/blob/feat-feed-article-markdown/packages/screens/FeedNewArticle/components/ArticleContentEditor/utils.ts#L14-L112