Creating and editing note experience should be the same

[galileo]

Release Type: Non-Breaking Feature

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

Description

We decided to follow recently with the path where we will align Notes experience which will only show us the plain textarea editor.

Checklist

Before submitting a pull request, please make sure the following is done:

• 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!

Steps to Test or Reproduce

1. Go into applicant
2. Add a new note (you can use markdown syntax)
3. Edit recent note, you should see only the plain textarea, when you select a word you should not see any context menu from Medium
4. Save

Impacted Areas in Application

Notes Inline editing, NotesFeed

Screenshots

[reyronald]

I think that given how we changed the component, the value argument should be renamed to event and set its type to SyntheticEvent<HTMLTextAreaElement>

[galileo]

Correct that the way which I followed on auto PR. Thanks 👍

[galileo]

@reyronald I applied changes but. After I added this Event definition for this method I ended up with multiple errors so I was forced to use in the event type definition
(value: any) to resolve them. I there is a possibility to solve this easily please let me know how to deal with that.

I guess the value can be anything right now from SyntheticEvent to any other object. You can try for example with mixed and it will crush for several places. I would really appreciate your help. Cheers

[reyronald]

That's because in the body of that function you are using .target instead of .currentTarget which is a common mistake, it's described in the Flow documentation here: https://flow.org/en/docs/react/events/

Note: To get the element instance, like HTMLButtonElement in the example above, it is a common mistake to use event.target instead of event.currentTarget. The reason why you want to use event.currentTarget` is that event.target may be the wrong element due to event propagation.

If you go directly to the Flow definition of SyntheticEvent in react-dom and check the target property, you'll even find more context:

https://github.com/facebook/flow/blob/2d9ade7e8bb97bbde458c69086c3460c3bcd04d9/lib/react-dom.js#L130-L132

declare class SyntheticEvent<+T: EventTarget = EventTarget, +E: Event = Event> {
  bubbles: boolean;
  cancelable: boolean;
  +currentTarget: T;
  defaultPrevented: boolean;
  eventPhase: number;
  isDefaultPrevented(): boolean;
  isPropagationStopped(): boolean;
  isTrusted: boolean;
  nativeEvent: E;
  preventDefault(): void;
  stopPropagation(): void;
  // This should not be `T`. Use `currentTarget` instead. See:
  // https://github.com/DefinitelyTyped/DefinitelyTyped/issues/11508#issuecomment-256045682
  +target: EventTarget;
  timeStamp: number;
  type: string;
  persist(): void;
}

A more thorough explanation of why we should use currentTarget instead is explained in that link: DefinitelyTyped/DefinitelyTyped#11508 (comment)

So, Flow's libdefs were designed to prevent this common mistake :). That's why it's always helpful to annotate things properly because then the tool will prevent us from doing unintended things.

With that fix it'll clear the errors you were getting related to that. After, you'll have to update the type of the onChange prop over at InputField too, but if you do it properly you'll get errors from other places of the app that are doing the same mistake, so I would suggest just using any there:

  onChange?: (value: any) => void,

It was untyped anyway before so you wouldn't be doing any more harm!

[reyronald]

I haven't tested it with those changes, but if applying them break the functionality you were aiming for just roll them back and disregard the suggestion, not worth it to spend more time debugging it just to fix that tiny thing 😅

[galileo]

Thanks a lot for the detailed answer I really appreciate it! I think I had the same thoughts while I was adding those changes. So the question is?
I added the definition for the onChange function like this:

 onChange?: (value: any) => mixed,

And you introduce the void return (actually is this the specification of return type for functions?)

 onChange?: (value: any) => void,

Should I change it to void? It was previously declared as mixed. WDYT? @reyronald

[nicksp]

According to official docs, () => mixed should be used for types that accept arbitrary functions. For our case when we have a defined function signature and it doesn't return anything, use of void should be fine. That's how I understand it though 😄

So, for this specific case, I'd suggest changing to void as it's more explicit type signature cc @galileo

[reyronald]

Just like Nick said, I guess () => mixed was chosen here so that it would accept arbitrary functions.

But in this case, event listeners are not really arbitrary functions, they all have the same base shape where they accept an Event as an argument and return nothing, so that's why in my snippet I changed it to onChange?: (value: any) => void. You can see how it's defined here:

https://developer.mozilla.org/en-US/docs/Web/API/EventListener/handleEvent (check "Return value")

And also the official specs (a little bit harder to interpret but you'll notice it doesn't return anything there either):

https://dom.spec.whatwg.org/#callbackdef-eventlistener

@galileo @nicksp

[nicksp]

Hey @galileo This looks very good now. I tested this in a browser locally and can confirm it behaves consistently, we no longer show a medium-formatting toolbar when selecting anything, we still preserve line breaks and highlight links. Good work!

Please just check several things related to the code. And check my input in this thread: #310 (comment)

[nicksp]

Since Function is unsafe and should be avoided(link), I'd use exactly () => mixed type for the onValueChange callback. Would like to see what you guys think.
cc @galileo @reyronald

[galileo]

But this is our general approach or from now because we are using a lot of Function definition, you can check in this file. For me it is misleading for this project if I can not base my assumption on the current project state 🤔

[nicksp]

Not everything we have in a project is correct apparently and we should fix it whenever possible. Once we're on this use case, let' fix it here.

[galileo]

Thanks for answering. Just to confirm. We, want to support for now both notations in the code?

[nicksp]

Not a change request, just some thoughts on code structure. For short methods that are 3-4 lines long, it's fine to have no empty lines. It's just when the method body is longer it's good to group logical blocks of code and separate them with empty lines.

[nicksp]

Over time we should eliminate all the use cases of unsafe types like Function, any, and Object. Since we can't do that at once, we're better focusing on fixing things in the area that we touch :)

[reyronald]

LGTM the way it is now @galileo, thanks for applying the changes!

[galileo]

Changes applied

[nicksp]

Latest changes look great to me :) Thanks Kamil! Happy to merge that now.