-
Notifications
You must be signed in to change notification settings - Fork 3
Article editor refactor for reuse and layout updates #2699
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
Conversation
for more information, see https://pre-commit.ci
merging with main branch
ahtesham-quraish
left a comment
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.
Your Changes look good to me!
|
@jonkafton there is bug in your code const [json, setJson] = useState<JSONContent>({
type: "doc",
content: article?.content
? JSON.parse(article.content)
: [{ type: "paragraph", content: [] }],
})You dont need to parse the content here, the content is already in json shap. 2 - Secondly you need to initialize the state properly. This should be const [json, setJson] = useState<JSONContent>({
type: "doc",
content: article?.content
? article.content
: [{ type: "paragraph", content: [] }],
})replace with following const [json, setJson] = useState<JSONContent>(article?.content || {
type: "doc",
content: [{ type: "paragraph", content: [] }],
})with your code it does not work for me I have to make changes locally to get this work |
|
After editing the article it should lead me to article detail page which is not happening RN |
|
There is one more issue related to initializing the editor state from existing article Currently we have in code. content: value || {
type: "doc",
content: [{ type: "paragraph", content: [] }],
},it should be replaced with this content: json || value || {
type: "doc",
content: [{ type: "paragraph", content: [] }],
},then it will show the article content. |
|
thanks @ahtesham-quraish . Not ready for review just yet |
We're not actually using the |
Yeah totally fine. |
OpenAPI ChangesShow/hide No detectable change.Unexpected changes? Ensure your branch is up-to-date with |
ahtesham-quraish
left a comment
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.
LGTM!
What are the relevant tickets?
N/A
Description (What does it do?)
Screenshots (if appropriate):
How can this be tested?
Additional Context