Implement RichTextEditor component in Note component

[reyronald]

Release Type: Non breaking feature

Fixes https://x-team-internal.atlassian.net/browse/XP-2673

Description

• Implement RichTextEditor component in Note component
• Add disabled prop to RichTextEditor

Checklist

• set yourself as an assignee
• set appropriate labels for a PR (In Review or In Progress depending on its status)
• move respective JIRA issue to the IN REVIEW column
• make sure your code lints (npm run lint)
• Flow typechecks passed (npm run flow)
• Snapshots tests passed (npm run jest)
• check cleanup tasks (https://github.com/x-team/auto/labels/cleanup) and take a suitable small one (if exists) in a related area of the current changes
• component's documentation (.stories.js file) is changed or added accordingly to reflect any new or updated use cases or variants usage.
• if you've fixed a bug or added code that should be tested, add unit tests
• if any snapshots have been changed, verify that component still works and looks as expected and update the changed snapshot
• manually tested the component by running it in the browser and checked nothing is broken and operates as expected!

Related PRs

branch	PR
XP-2673-rich-text-editor-in-notes	Auto

Steps to Test or Reproduce

See https://github.com/x-team/auto/pull/1761

Impacted Areas in Application

• Note component (used when editing notes from the applicant's profile)

Screenshots

Disabled mode for rich text editor

Note component with new rich text editor

See https://github.com/x-team/auto/pull/1761

[galileo]

Just wondering what was the reason behind adding the disabled flag? I think it is really usefull but did you find any current use case for it?

[galileo]

Just, wanted to ask. Why this is not working from the beginning? This is because the RichTextEditor is initialized after the React component is rendered the first time, and we need to forceUpdate it?
What I don't get it is that we are adding the disabled property on the initialization of that component so it should be built with it.
I know that it is necessary because I tried this code, but would appreciate the clarification for the why this is not rendering at the beginning, this will help me understand react more.

[galileo]

I guess was able to grasp the way. Actually, it is rendering but the size is different and it is rendering small and then we need to re-render to adjust the size of that overlay? Is this correct @reyronald?

[reyronald]

That's correct, as you can see in the render method, I am using just a regular div to create a transparent overlay on top of the editor when in disabled mode, and I need this div to be the same size of the editor itself, but on the first render cycle of the component the editor is not initialized yet and it's not even present in the DOM, so the size would be 0.

After I initialize the editor, then I need the component to re-render so that the overlay div picks up the correct size. We usually trigger re-renders in React by calling setState, but in this case there's no state for us to update because the TUI Editor doesn't depend on the current components state, it handles its own state internally. Here I use a this.forceUpdate to get around this.

Let me know if that wasn't clear enough. @galileo

[reyronald]

Just wondering what was the reason behind adding the disabled flag? I think it is really usfull but did you find any current use case for it?

I forgot to answer this, it's because the NewNote component disables the editor in some cases:

        <div className={cx.bodyArea}>
          <Avatar src={author.photo} size={40} />
          <div className={cx.input} data-testid='newnote-note'>
            <InputField
              type='textarea'
              placeholder={`Type a ${capitalizedTargetType} Note here or click the tabs above to write a different Note.`}
              value={body}
              onChange={this.handleContentChange}
              linesLimit={10}
              disabled={noteId}
            />
          </div>
        </div>

https://github.com/x-team/auto/blob/d4f315536f8f5fa58684618db6f47f91b768fdba/src/client/components/NewNote.js#L405

[galileo]

Great Thanks for the all informations. This is what I thought why this is done this way after I dig deeper in the code. So for the edit note it will be only disabled when we will try to use this component without note, which will have an noteId?

[reyronald]

@galileo To be honest I couldn't reproduce a case where the editor would be disabled in this NewNote component. I think that it might be an old case that could happen before but doesn't anymore. I took a note of it with the intention of looking into it later in the week and possibly create a cleanup issue to remove that. But I included the disabled prop anyway because it is indeed useful regardless!

Perhaps @nicksp remembers or can tell us how we can trigger this disabled behavior in this component.

[galileo]

Why we are using line-height: 1, in this place? Is this a common approach in our app? Should not be the line-height a little bit bigger than the font-size?

[nicksp]

This place looks fine. line-height is the default size of the line-height in the browser

[reyronald]

Good question. In general, the line-height doesn't have to be bigger than the font-size for it work, a value of 1 would mean that the there's no additional blank space on top and below of the text on each line. In our app specifically (as you may know) we've configured the typography globally to have a slightly higher line-height, just to make it look cleaner I suppose.

But the TUI Editor was designed to work with line-height of 1 apparently, and when I integrated the editor into auto I noticed that the line-height of the app or of an outer-component bleeds into the styles of the editor and this would happen:

Setting the line-height to 1 here we override outer styles and fix that issue:

@galileo

[galileo]

Great answer @reyronald. That is exactly what I was curious about, thanks for this explanation. Awesome 5️⃣ !!

[bernardodiasc]

LGTM 👍 Tested on my local and works as expected.

[nicksp]

LGTM!

[nicksp]

In React realm, we should strive to not use Objects, Arrays, inline functions as prop values cause that will get a different instance on each re-render thus causing useless re-renders of all children. This even the case with PureComponents 'cause React only does a shallow comparison of this.props so a pure component will be rerendered also.

This is a good habit to keep in mind. I don't request to change that now, but please remember to define separate variables for reference values next time.

[reyronald]

I don't think this is applicable here because the dimensions have to be recomputed on every render, instance variables are only computed at component instantiation time so that wouldn't be the solution. Declaring a separate variable on the scope of the render would be the same thing that I am doing here so that's not it either.

To avoid unnecessary re-render in this case what would be necessary is a memoization function that memoizes the dimension object given a width and a height, so it would be a little more than just defining a separate variable.

---

Also I would like to add that creating new reference values on every render is not as bad as most of the community thinks, and some times it could even be better, here's a blog post that talks about that by the co-author of react-router and a bunch of other stuff: https://cdb.reacttraining.com/react-inline-functions-and-performance-bdff784f5578.

It's interesting too to note that the Hooks API that's upcoming this month in React basically completely ignores this issue, and that's how the React team is recommending the community to write components from now on. It's because they've noticed that the added complexity at code level to get around this is not worth the micro-optimization in most cases, and this has been my experience as well. So when I do this it's not because I forgot, it's because I decided to 🙈 , but feel free to request changes for that stuff in the future if it's a concern to you!

[nicksp]

I don't think this is applicable here because the dimensions have to be recomputed on every render

That's what I thought might be the case, so didn't request the change, but wanted to bring up the point of rendering optimizations 😄 And is a use case to write it like that, I agree in this particular case.

Declaring a separate variable on the scope of the render would be the same thing that I am doing here so that's not it either.

That's for sure, will be the same 😆

Also I would like to add that creating new reference values on every render is not as bad as most of the community thinks

For this kind of things I always trust performance measurements that I run. That's how it ideally should be when optimizing: you first find weak slow parts, then optimize and measure, so we can compare performance metrics. So, it's fine to have it like that in this particular case given a specific use case 👍

[nicksp]

And even though it might not be that bad, I'd still keep away from using reference values as props, because of 2 things:

1. useless re-renders of children
2. creating new instances in memory, so in case of numerous re-renders we keep excessive data in memory.

But, as I said in last message, before making any changes, ideally we're better to run performance checks so there's a clear way to see a difference from applying any kind of optimizations.

[galileo]

Great discussion guys. Really like it. Can you provide some valid practicies how to measure the performance of react apps. I can search for something over the internet but would like to get an recommendation from you if that is possible.

[reyronald]

Oh, and then this lesson on egghead:

Use the New Profiler in React Developer Tools to Generate Flame Charts and Interactions

There are many other good lessons and courses on egghead I won't link them all here, but just search for "optimize", "optimization", "profiler", etc.

[nicksp]

@galileo Please note that React Profiler is still experimental and is available only in latest versions. There're some more solid options for measuring performance. Some of the recommendations I found useful and that works in more versions of React:

• https://engineering.musefind.com/how-to-benchmark-react-components-the-quick-and-dirty-guide-f595baf1014c
• https://calibreapp.com/blog/react-performance-profiling-optimization/ (this one, in particular, is really great manual)
• https://www.keycdn.com/blog/react-performance
• https://www.youtube.com/watch?v=nl8VVig_9aM
• https://www.youtube.com/watch?v=b8IcYOV5_Rc

[reyronald]

I'd still keep away from using reference values as prop

I think we might be using the wrong term here, it's not reference values that cause a useless re-render of children (in fact everything that's not a primitive value is passed as a reference value), the issue is passing a new or different reference on each render, but not the fact that it's a reference.

I also forgot to mention that in this case even if we are creating a new style object on every render this doesn't trigger additional re-renders of any children components, because this reference value is being applied to a native div element , which is not a React component and therefore doesn't implement a render method. It doesn't have any children either and even if it did, the children are not going to be replaced on the DOM, only the parent div which is receiving the new attributes, this is React diffing algorithm kicking in.

So this already as optimal as it can be, and even memoization wouldn't actually do any difference so forget I mentioned that (for this case). The statement new reference value cause re-render is not always true, it depends on the case and how/where it is applied.

[reyronald]

the children are not going to be replaced on the DOM, only the parent div which is receiving the new attributes

Correction: The parent element is not going to be replaced either, it's just going to be updated with the new attributes.

[reyronald]

Please note that React Profiler is still experimental and is available only in latest versions

Profiler was introduced in v16.5 stable release 4 months ago, latest React version is v16.7.0 and we have v16.6.3 here.

I haven't checked those links, certainly would if I was working with an older version of React, but otherwise I would prefer to use the official solution by Facebook unless I bump into some trouble with it.

[nicksp]

Sure, i just wasn’t super precise. OFC we’re talking about values that are new instances on each render phase, that wad clear I supposed ;) Sent from myMail for iOS Friday, 11 January 2019, 20:17 +0300 from notifications@github.com <notifications@github.com>:

…

@reyronald commented on this pull request. ---------------------------------------------------------------------- In src/components/ui/RichTextEditor.js : > - return <div className={cx.root}> - <div ref={this.editSection} /> - {characterLimit !== Infinity && ( - <div className={cx.characterCounter}>{characterCount}/{characterLimit}</div> - )} - </div> + const { disabled, characterLimit } = this.props + + const { offsetWidth, offsetHeight } = this.editSection.current || {} + + return ( + <div className={cx.root}> + + {disabled && <div + className={cx.disabledOverlay} + style={{ >I'd still keep away from using reference values as prop I think we might be using the wrong term here, it's not reference values that cause a useless re-render of children (in fact everything that's not a primitive value is passed as a reference value), the issue is passing a new or different reference on each render, but not the fact that it's a reference. I also forgot to mention that in this case even if we are creating a new style object on every render this doesn't trigger additional re-renders of any children components , because this reference value is being applied to a native div element , which is not a React component and therefore doesn't implement a render method. It doesn't have any children either and even if it did, the children are not going to be replaced on the DOM, only the parent div which is receiving the new attributes, this is React diffing algorithm kicking in. So this already as optimal as it can be, and even memoization wouldn't actually do any difference so forget I mentioned that (for this case). The statement new reference value cause re-render is not always true, it depends on the case and how/where it is applied. — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub , or mute the thread .

[nicksp]

Exactly cause React knows if that’s a native element it can use DOM api to update the look and thus remounts is unneeded.  Sent from myMail for iOS Friday, 11 January 2019, 20:19 +0300 from notifications@github.com <notifications@github.com>:

…

@reyronald commented on this pull request. ---------------------------------------------------------------------- In src/components/ui/RichTextEditor.js : > - return <div className={cx.root}> - <div ref={this.editSection} /> - {characterLimit !== Infinity && ( - <div className={cx.characterCounter}>{characterCount}/{characterLimit}</div> - )} - </div> + const { disabled, characterLimit } = this.props + + const { offsetWidth, offsetHeight } = this.editSection.current || {} + + return ( + <div className={cx.root}> + + {disabled && <div + className={cx.disabledOverlay} + style={{ >the children are not going to be replaced on the DOM, only the parent div which is receiving the new attributes Correction: The parent element is not going to be replaced either, it's just going to be updated with the new attributes. — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub , or mute the thread .

[nicksp]

I use another off solution described in links i shared ?react_perf query param.  Sent from myMail for iOS Friday, 11 January 2019, 20:26 +0300 from notifications@github.com <notifications@github.com>:

…

@reyronald commented on this pull request. ---------------------------------------------------------------------- In src/components/ui/RichTextEditor.js : > - return <div className={cx.root}> - <div ref={this.editSection} /> - {characterLimit !== Infinity && ( - <div className={cx.characterCounter}>{characterCount}/{characterLimit}</div> - )} - </div> + const { disabled, characterLimit } = this.props + + const { offsetWidth, offsetHeight } = this.editSection.current || {} + + return ( + <div className={cx.root}> + + {disabled && <div + className={cx.disabledOverlay} + style={{ >Please note that React Profiler is still experimental and is available only in latest versions Profiler was introduced in v16.5 stable release 4 months ago, latest React version is v16.7.0 and we have v16.6.3 here. I haven't checked those links, certainly would if I was working with an older version of React, but otherwise I would prefer to use the official solution by Facebook unless I bump into some trouble with it. — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub , or mute the thread .