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

feat: Upstream Trix/RichTextField. #90

Merged
merged 9 commits into from
Jun 1, 2021
Merged

feat: Upstream Trix/RichTextField. #90

merged 9 commits into from
Jun 1, 2021

Conversation

stephenh
Copy link
Contributor

~75% of is from my hack day where I needed a BoundRichTextField in internal-frontend and so adapted/converted the BP Trix from the original class-based code we'd copy/pasted from a react-trix wrapper over into hooks.

Also a first pass at Beam-ish styling, i.e. using our border radius, focus color, etc.

@stephenh stephenh requested a review from alexkuang May 28, 2021 15:12
@stephenh stephenh changed the title Upstream Trix/RichTextField. feat: Upstream Trix/RichTextField. May 28, 2021
import Tribute from "tributejs";
import "tributejs/dist/tribute.css";
import "trix/dist/trix";
import "trix/dist/trix.css";
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 :-/

vitejs/vite#3409

Copy link
Contributor

Choose a reason for hiding this comment

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

Surprising!

Copy link
Contributor

@KoltonG KoltonG left a comment

Choose a reason for hiding this comment

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

🍣 awesome @stephenh !!

@@ -0,0 +1,152 @@
import { Global } from "@emotion/react";
import * as React from "react";
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: Is this still required with our new setup in Beam?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's only for the React.createElement line down below which is how the original code that we copy/pasted created the <trix-editor> custom HTML element / web component thingy.

Copy link
Contributor

Choose a reason for hiding this comment

The 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 createElement via a named export on line 3?

Copy link
Contributor Author

@stephenh stephenh May 28, 2021

Choose a reason for hiding this comment

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

Oh sure, I was too wrote-Java-for-a-decade thinking that React.createElement was "a static method on the React class", but we can import { createElement, ... }. Yep, changed.

import Tribute from "tributejs";
import "tributejs/dist/tribute.css";
import "trix/dist/trix";
import "trix/dist/trix.css";
Copy link
Contributor

Choose a reason for hiding this comment

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

Surprising!


export default {
component: RichTextEditor,
title: "Components/Rich Text Editor",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
title: "Components/Rich Text Editor",
title: "Components/Rich Text Editors",

Adding this will merge the parent story name and the child story into one.
CleanShot 2021-05-28 at 12 36 39@2x

Copy link
Contributor Author

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.

Copy link
Contributor

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 before

Copy link
Contributor Author

@stephenh stephenh May 28, 2021

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.

{/* TODO: Not sure what to pass to labelProps. */}
{label && <Label labelProps={{}} label={label} />}
<div css={trixCssOverrides}>
{React.createElement("trix-editor", {
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious about this approach vs <TrixEditor /> usage (if I am understanding this right)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The <TrixEditor /> in BP is actually our own src/components/Trix.sx that is doing this same createElement(trix-editor) line. (Which is confusing, the <TrixEditor /> doesn't "look like our thing". I renamed it here to <RichTextField /> which I think will be less confusing.)

return (
<div css={Css.w100.maxw("550px").$}>
{/* TODO: Not sure what to pass to labelProps. */}
{label && <Label labelProps={{}} label={label} />}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that is a React-Aria label thingy?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, they come back from hooks like useTextField but I wasn't sure what look to use here. useTextField returns both labelProps and inputProps but the inputProps specifically want to be on an input type=text and not the input type=hidden. ...not really sure.

);
}

function attachTributeJs(mergeTags: string[], editorElement: HTMLElement) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: Wondering if we could return the tribute object and possibly add useEffect in the component above listening on the mergeTags and use https://www.npmjs.com/package/tributejs#updating-a-collection-with-new-data to real-time update the tags? Not sure if that was attempted before?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 Menu and useOverlay to do this native and drop tribute all together at some point.


function attachTributeJs(mergeTags: string[], editorElement: HTMLElement) {
const values = mergeTags.map((value) => ({ value }));
const tribute = new Tribute({
Copy link
Contributor

Choose a reason for hiding this comment

The 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 😋

} catch {}
}

const trixCssOverrides = {
Copy link
Contributor

Choose a reason for hiding this comment

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

💯 awesome!

@stephenh stephenh merged commit 24fdcf5 into main Jun 1, 2021
@stephenh stephenh deleted the rich-text-editor branch June 1, 2021 13:02
homebound-team-bot pushed a commit that referenced this pull request Jun 1, 2021
## [1.32.0](v1.31.0...v1.32.0) (2021-06-01)

### Features

* Upstream Trix/RichTextField. ([#90](#90)) ([24fdcf5](24fdcf5))
@homebound-team-bot
Copy link

🎉 This PR is included in version 1.32.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants