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

Rich text posts #892

Closed
wants to merge 5 commits into from
Closed

Rich text posts #892

wants to merge 5 commits into from

Conversation

richardgirges
Copy link
Contributor

This is the first draft of rich text editing with CK Editor.

Things to note:

@CarsonF
Copy link
Member

CarsonF commented Jul 15, 2021

We've got a few libraries we provide our own types for in the typings folder. It looks like there are types in DefinitelyTyped for the core of the library, so really we just need to provide connection points to this library. Should be trivial since both referenced are thin wrappers.

CarsonF
CarsonF approved these changes Jul 19, 2021
View reviewed changes
Copy link
Member

@CarsonF CarsonF left a comment

Choose a reason for hiding this comment

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

Looks good @richardgirges!

@ParkeBrown ParkeBrown added this to the Sprint 22 FY21 milestone Jul 20, 2021
@richardgirges richardgirges force-pushed the posts-richtext branch 3 times, most recently from 828c023 to b7e65f0 Compare August 3, 2021 04:12
@CarsonF CarsonF force-pushed the posts-richtext branch 2 times, most recently from 46ac37d to 356bd95 Compare August 5, 2021 17:50
CarsonF
CarsonF requested changes Aug 9, 2021
View reviewed changes
Copy link
Member

@CarsonF CarsonF left a comment

Choose a reason for hiding this comment

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

The CKE features should be limited to only those we can render. I'd remove quotes, tables, pics, and video.

We should style the output with react-html-parser and the display inside of the editor to be consistent with our theme.

src/components/posts/PostListItemCard/PostListItemCard.tsx Outdated
@@ -124,7 +127,7 @@ export const PostListItemCard: FC<PostListItemCardProps> = ({
</Typography>
</div>
</div>
<Typography variant="body2">{post.body.value}</Typography>
<Typography variant="body2">{postBody}</Typography>
Copy link
Member

Choose a reason for hiding this comment

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

Currently this puts any html tags under a <p> tag which is invalid HTML.

@CarsonF CarsonF removed their assignment Aug 9, 2021
@ParkeBrown
Copy link

Richard: could go live if needed with just what is PR'd; available next in Dec

@ParkeBrown
Copy link

@daniel-leod can you confirm that we still need to address the unresolved comment above?

@daniel-leod
Copy link
Contributor

@daniel-leod can you confirm that we still need to address the unresolved comment above?

@ParkeBrown I think the last commit would resolve that. This PR is ready for review.

@daniel-leod daniel-leod requested a review from CarsonF November 9, 2021 22:20
@ParkeBrown
Copy link

Daniel believes he's covered the requested changes

@CarsonF
Copy link
Member

CarsonF commented Jul 7, 2022

I'd like to go with an editor that produces JSON instead, instead of html string. So rendering can be easily customized based on context.

@CarsonF CarsonF closed this Jul 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michaelmarshall michaelmarshall Awaiting requested review from michaelmarshall

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

Assignees

@daniel-leod daniel-leod

Labels
None yet
Projects
None yet
Milestone
Sprint 5 FY22
Development

Successfully merging this pull request may close these issues.
4 participants
@richardgirges @CarsonF @ParkeBrown @daniel-leod