Add Block: Chat #1632
Add Block: Chat #1632
Conversation
|
OK, so the last build failed because of a bus error. A restart would help. Also, I just noticed that I can't properly edit an existing chat block now, because the textarea will show all the markup. I assume I need to change this to an |
| import { uniq } from 'lodash'; | ||
|
|
||
| export default function ChatTranscript( props ) { | ||
| const { value, compact } = props; |
There was a problem hiding this comment.
You can deconstruct directly inside the function declaration.
| message = msg.join( ':' ).trim(); | ||
|
|
||
| lastAuthor = getAuthor( author ); | ||
| } |
There was a problem hiding this comment.
Do you think we could split the logic of this into two parts?
// Normalize data
const transcript = normalizeTranscript( value ); // returns { authors: [], messages: [ { message, authorIndex } ] }
// Render
return messages.map( ... )This would make unit-testing the normalizing logic easier.
blocks/library/chat/index.js
Outdated
| /** | ||
| * External dependencies | ||
| */ | ||
| import TextareaAutosize from 'react-autosize-textarea'; |
There was a problem hiding this comment.
For consistency, should we move the external dependencies above?
blocks/library/chat/index.js
Outdated
| </Placeholder> | ||
| ), | ||
| ! focus && ( | ||
| <ChatTranscript value={ content } compact={ compact } /> |
There was a problem hiding this comment.
We should probably use the className of the block here (for the root element)
blocks/library/chat/style.scss
Outdated
| @@ -0,0 +1,20 @@ | |||
| .editor-visual-editor__block[data-type="core/chat"] { | |||
There was a problem hiding this comment.
Should we rely on the base classname instead? wp-block-chat
There was a problem hiding this comment.
From what I've seen, other blocks use .editor-visual-editor__block… in style.scss and wp-block-… in block.scss, so I chose to follow that pattern.
There was a problem hiding this comment.
I think we should always use wp-block- unless we need to change something in the parent wrapper (which is unlikely to happen, it's needed for float alignments essentially)
| min-height: 10em; | ||
| text-align: left; | ||
| } | ||
| } |
There was a problem hiding this comment.
Should we create a block.scss for frontend styling too, and extract the common styles applied for the editor and for frontend into the block.scss file?
There was a problem hiding this comment.
There already is such a file or am I missing something?
There was a problem hiding this comment.
Each block should create its own block.scss (which are extracted and loaded in frontend too). See #1590 (we did this for other blocks)
There was a problem hiding this comment.
Yeah, that's why blocks/library/chat/block.scss exists. What else is needed?
| Okay, that's enough. No internet for you anymore! | ||
| Alexa: 💔 | ||
| Google Home: 😔 | ||
| Siri: 📴 |
There was a problem hiding this comment.
I think there's a problem here unless I'm missing something.
If I understand the code, this value will be parsed and reserialized differently (using HTML ...).
What happens if I try to re-edit the block again? The parser will fail?
Anyway, The source value and the serialized value should be the same (aside some spacing differences maybe) which makes me think we should store the raw value in the comment.
There was a problem hiding this comment.
Yeah, that's a problem I still need to address, see #1632 (comment).
You can edit the block, but you will see all the HTML markup in the textarea.
I would love to store only the raw value, but then how can it be rendered on the front end?
Feedback last week was that it should be stored with (as little as possible) markup in the database and then we'll see from there.
There was a problem hiding this comment.
Ok Maybe instead of storing the value in its currenf format, we should extract a more "normalized" value, something like:
content: query( 'p', {
authorId: attr( 'class' ),
author: html( '.chat-author' ),
message: hmlt( '.chat-message' ),
} )which I think should return something like:
const content = [
{ authorId, author, message }
];And when typing in the text area, parsing the textarea value to the format above before calling the setAttributes
| value.split( '\n' ).map( ( line ) => { | ||
| line = line.trim(); | ||
|
|
||
| const messageParts = line.split( ':' ); |
There was a problem hiding this comment.
This should add a limit param, so like line.split( ':', 2 ). Otherwise it will choke on a line like:
John: I really like the ratio 1:1.
There was a problem hiding this comment.
Then the message = messageParts.join( ':' ).trim() part below can be replaced with:
message = messageParts.shift().trim()
| /** | ||
| * WordPress dependencies | ||
| */ | ||
| import { Placeholder } from 'components'; |
There was a problem hiding this comment.
Note with #2172, these need to be updated to prefix the dependencies with @wordpress/. You will need to perform a rebase against the latest version of master and apply your changes:
git fetch origin
git rebase origin/master
|
Can we either close this or update the PR please? |
|
Since #954 was closed, I guess this PR can be closed as well. It's far from done anyway (compared to the pagination block PR, for example). |
|
@swissspidy, let's close it as you said. Feel free to reopen as soon as you get back to this one. Thanks for spending time exploring this block 👍 |
Just like in the mockups, I added different colors for various speakers / chat participants. The whole parsing is super simple for now. Perhaps that would need to be improved in the future to account for different kinds of chat transcripts (e.g. containing time stamps and whatnot).
I also added a "compact view" toggle that makes the whole chat transcript a bit more … compact.
Feedback welcome.
Screenshots:
Fixes #954.