-
Notifications
You must be signed in to change notification settings - Fork 2
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: Upstream Trix/RichTextField. #90
Changes from 3 commits
ab3a82c
4ac2d4d
1ff0103
691311a
69ec9c0
a3a0602
fedf3ee
454282f
ba67a5e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
import { Meta } from "@storybook/react"; | ||
import { useState } from "react"; | ||
import { RichTextEditor } from "src/components/RichTextEditor"; | ||
|
||
export default { | ||
component: RichTextEditor, | ||
title: "Components/Rich Text Editor", | ||
} as Meta; | ||
|
||
export function RichTextEditors() { | ||
return <TestEditor />; | ||
} | ||
|
||
function TestEditor() { | ||
const [value, setValue] = useState(""); | ||
return ( | ||
<> | ||
<RichTextEditor label="Comment" value={value} onChange={setValue} mergeTags={["foo", "bar", "zaz"]} /> | ||
value: {value} | ||
</> | ||
); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,152 @@ | ||
import { Global } from "@emotion/react"; | ||
import * as React from "react"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Question: Is this still required with our new setup in Beam? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's only for the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since the line below is already importing React what about we merge them and/or take out the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh sure, I was too wrote-Java-for-a-decade thinking that |
||
import { ChangeEvent, useEffect, useRef } from "react"; | ||
import { useId } from "react-aria"; | ||
import { Label } from "src/components/Label"; | ||
import { Css, Palette } from "src/Css"; | ||
import Tribute from "tributejs"; | ||
import "tributejs/dist/tribute.css"; | ||
import "trix/dist/trix"; | ||
import "trix/dist/trix.css"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We still have an issue with vitejs where these "css imports that are done in a dependency" don't work :-/ There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Surprising! |
||
|
||
export interface RichTextEditorProps { | ||
/** The initial html value to show in the trix editor. */ | ||
value: string; | ||
onChange: (html: string, text: string) => void; | ||
/** | ||
* A list of tags/names to show in a popup when the user `@`-s. | ||
* | ||
* Currently we don't support mergeTags being updated. | ||
*/ | ||
mergeTags?: string[]; | ||
label?: string; | ||
autoFocus?: boolean; | ||
placeholder?: string; | ||
} | ||
|
||
// There aren't types for trix, so add our own. For now `loadHTML` is all we call anyway. | ||
type Editor = { | ||
loadHTML(html: string): void; | ||
}; | ||
|
||
/** | ||
* Glues together trix and tributejs to provide a simple rich text editor. | ||
* | ||
* See [trix]{@link https://github.com/basecamp/trix} and [tributejs]{@link https://github.com/zurb/tribute}. | ||
* */ | ||
export function RichTextEditor(props: RichTextEditorProps) { | ||
const { mergeTags, label, value, onChange } = props; | ||
const id = useId(); | ||
|
||
// We get a reference to the Editor instance after trix-init fires | ||
const editor = useRef<Editor | undefined>(undefined); | ||
|
||
// Keep track of what we pass to onChange, so that we can make ourselves keep looking | ||
// like a controlled input, i.e. by only calling loadHTML if a new incoming `value` !== `currentHtml`, | ||
// otherwise we'll constantly call loadHTML and reset the user's cursor location. | ||
const currentHtml = useRef<string | undefined>(undefined); | ||
|
||
useEffect(() => { | ||
const editorElement = document.getElementById(`editor-${id}`); | ||
if (!editorElement) { | ||
throw new Error("editorElement not found"); | ||
} | ||
|
||
editor.current = (editorElement as any).editor; | ||
if (!editor.current) { | ||
throw new Error("editor not found"); | ||
} | ||
if (mergeTags !== undefined) { | ||
attachTributeJs(mergeTags, editorElement!); | ||
} | ||
// We have a 2nd useEffect to call loadHTML when value changes, but | ||
// we do this here b/c we assume the 2nd useEffect's initial evaluation | ||
// "missed" having editor.current set b/c trix-initialize hadn't fired. | ||
editor.current.loadHTML(value); | ||
|
||
function trixChange(e: ChangeEvent) { | ||
const { textContent, innerHTML } = e.target; | ||
currentHtml.current = innerHTML; | ||
onChange && onChange(innerHTML, textContent || ""); | ||
} | ||
|
||
editorElement.addEventListener("trix-change", trixChange as any, false); | ||
return () => { | ||
editorElement.removeEventListener("trix-change", trixChange as any); | ||
}; | ||
}, []); | ||
|
||
useEffect(() => { | ||
// If our value prop changes (without the change coming from us), reload it | ||
if (editor.current && value !== currentHtml.current) { | ||
editor.current.loadHTML(value); | ||
} | ||
}, [value]); | ||
|
||
const { placeholder, autoFocus } = props; | ||
|
||
return ( | ||
<div css={Css.w100.maxw("550px").$}> | ||
{/* TODO: Not sure what to pass to labelProps. */} | ||
{label && <Label labelProps={{}} label={label} />} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that is a React-Aria label thingy? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, they come back from hooks like |
||
<div css={trixCssOverrides}> | ||
{React.createElement("trix-editor", { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Curious about this approach vs There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The |
||
id: `editor-${id}`, | ||
input: `input-${id}`, | ||
...(autoFocus ? { autoFocus } : {}), | ||
...(placeholder ? { placeholder } : {}), | ||
})} | ||
<input type="hidden" id={`input-${id}`} value={value} /> | ||
</div> | ||
<Global styles={[tributeOverrides]} /> | ||
</div> | ||
); | ||
} | ||
|
||
function attachTributeJs(mergeTags: string[], editorElement: HTMLElement) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Question: Wondering if we could return the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, we could do that, just hasn't been necessary yet. I copy/pasted this over and hopefully this will be fine for awhile, but it might be worth using a beam |
||
const values = mergeTags.map((value) => ({ value })); | ||
const tribute = new Tribute({ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Pretty neat that you could include images here too! https://www.npmjs.com/package/tributejs#how-do-i-add-an-image-to-the-items-in-the-list but not sure how it will look 😋 |
||
trigger: "@", | ||
lookup: "value", | ||
allowSpaces: true, | ||
/** {@link https://github.com/zurb/tribute#hide-menu-when-no-match-is-returned} */ | ||
noMatchTemplate: () => `<span style:"visibility: hidden;"></span>`, | ||
selectTemplate: ({ original: { value } }) => `<span style="color: ${Palette.LightBlue700};">@${value}</span>`, | ||
values, | ||
}); | ||
// In dev mode, this fails because jsdom doesn't support contentEditable. Note that | ||
// before create-react-app 4.x / a newer jsdom, the trix-initialize event wasn't | ||
// even fired during unit tests anyway. | ||
try { | ||
tribute.attach(editorElement!); | ||
} catch {} | ||
} | ||
|
||
const trixCssOverrides = { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💯 awesome! |
||
...Css.relative.add({ wordBreak: "break-word" }).$, | ||
// Put the toolbar on the bottom | ||
...Css.df.flexColumnReverse.childGap1.$, | ||
// Some basic copy/paste from TextFieldBase | ||
"& trix-editor": Css.bgWhite.sm.gray900.br4.bGray300.$, | ||
"& trix-editor:focus": Css.bLightBlue700.$, | ||
// Make the buttons closer to ours | ||
"& .trix-button": Css.bgWhite.sm.$, | ||
// We don't support file attachment yet, so hide that control for now. | ||
"& .trix-button-group--file-tools": Css.dn.$, | ||
// Other things that are unused and we want to hide | ||
"& .trix-button--icon-heading-1": Css.dn.$, | ||
"& .trix-button--icon-code": Css.dn.$, | ||
"& .trix-button--icon-quote": Css.dn.$, | ||
"& .trix-button--icon-increase-nesting-level": Css.dn.$, | ||
"& .trix-button--icon-decrease-nesting-level": Css.dn.$, | ||
"& .trix-button-group--history-tools": Css.dn.$, | ||
// Put back list styles that CssReset is probably too aggressive with | ||
"& ul": Css.ml2.add("listStyleType", "disc").$, | ||
"& ol": Css.ml2.add("listStyleType", "decimal").$, | ||
}; | ||
|
||
// Style the @ mention box | ||
const tributeOverrides = { | ||
".tribute-container": Css.add({ minWidth: "300px" }).$, | ||
".tribute-container > ul": Css.sm.bgWhite.ba.br4.bLightBlue700.overflowHidden.$, | ||
}; |
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.
Adding this will merge the parent story name and the child story into one.
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.
Oh, I get why you were doing this now. Cool, will change.
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.
Yeah! For single stories I usually opt for the singular title and do the
as
approach like you saw beforeThere 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 really wish they would base this "merge parent story and child story" behavior off of "there is only 1 export'd function" instead of "the function name matches the file name", given that we will essentially always a conflict between the component-under-test name and that story file name.