Message syntax highlighting #4759
Conversation
Generated by 🚫 dangerJS |
| import { convertFromRaw, convertToRaw, EditorState } from 'draft-js'; | ||
| import { addEmbedsToEditorState } from './add-embeds-to-draft-js'; | ||
|
|
||
| export default (type: 'TEXT' | 'DRAFTJS', body: string): string => { |
There was a problem hiding this comment.
The type argument here should be lower case to match the input message.messageType.
That way, you do not have to have a conditional outside of this method and can simply do
message.content.body = processMessageContent(message.messageType, message.content.body);
message.messageType = 'draftjs'There was a problem hiding this comment.
To be honest, I just copied most of this file from here: https://github.com/withspectrum/spectrum/blob/alpha/shared/draft-utils/process-thread-content.js#L6 and removed one property that was irrelevant for messages. Could have done this refactor better probably, but I just didn't want to change the already existing naming of the stuff. If I change this here, it should be changed everywhere else too.
The type argument / property for thread-related stuff is uppercase in the code. Is it lowercase like this anywhere else? Otherwise I'd suggest just uppercasing it here for the sake of consistency.
There was a problem hiding this comment.
It is inconsistent in the database, but it should be consistent with that. Threads have a type property that is either TEXT or DRAFTJS, but messages have a messageType property that is either text or draftjs.
That is inconsistent with each other, which is bad, but this method shiuld be consistent with the database, if that makes sense?
There was a problem hiding this comment.
Oh, I was not aware of that, I guess. Will change it then.
There was a problem hiding this comment.
Yep I figured, that's why I left that review comment. Sorry for not giving the full context in the initial comment!
There was a problem hiding this comment.
Okay, I committed some changes to fix it. While on it, I also noticed that those strings ('text' and 'draftjs') and so on, are all over the code, so I made some sort of "enum" for it and replaced all occurencies I could find using that. Strings are prone to typos, this should avoid it for future use. I can revert that and just fix the above, if that's required, but I think it's a good addition.
There was a problem hiding this comment.
This is great, thanks for making that more consistent! 💯 I think we can ship this, will defer to @brianlovin for a final review
…-syntax-highlighting
…-syntax-highlighting
|
@lookapanda this is really good, but is it possible for us to preserve the language selection when editing a message? If I type a message with a js-highlighted code block, then edit it and save the edit, the js highlighting goes away. |
|
Let me check |
|
Okay, I figured out how it works. I needed to modify the |
Will let Max weigh in on this! |
|
Rather than forking it submit the change as a PR to Also, that fix really has nothing to do with this PR, it should be done regardless! Let's submit the patch as an upstream PR, revert the commit here and upgrade the dep in a spearate PR once the change is merged and published. |
This reverts commit d36c044.
|
So, I should do a PR at that project? |
|
Yes, that one is perfect! e2e test failure is unrelated, something to do with the new composer (cc @brianlovin), so I am going to ship this. Thanks so much for the great contribution @lookapanda! |
3 participants
Status
Deploy after merge (delete what needn't be deployed)
Release notes for users (delete if codebase-only change)
Related issues (delete if you don't know of any)
Closes #4677