Display error message when button in email still has its default text #2451
Conversation
There was a problem hiding this comment.
Nice work! I have looked at the preview build and I have reviewed the code changes.
I was super happy to see this. I'm hoping to be able to make a release sometime next week, but there's not much to release, so this would be lovely to merge. 😊
I did find two things that I think could be improved, one of which is related to the problem you raised in the PR:
Neither version allows for the button text to consist of zero characters, but as a result of this PR, the error message reads 'This button still has the default text' also in the case of an empty (0 characters) button.
| data.buttonText | ||
| ? messages.editor.tools.button.title() | ||
| : messages.editor.tools.button.settings.defaultButtonTextWarning() |
There was a problem hiding this comment.
I was thinking that maybe the error message could be moved out of the header? I mean the block is still a button even if it's invalid, so I think it might be a bit jarring to replace the title like this.
Compare this with how invalid links are handled in paragraph/text blocks. It's still a text block and titled as such ("Text"), but there's an additional warning underneath.
There was a problem hiding this comment.
Thank you! Of course that would be a more suitable way to handle it. I'll be back with an updated version shortly.
There was a problem hiding this comment.
It seems to be doing what I intended for it to do: displays error when the button has its default text, when the button has zero characters and when the button text consists of only whitespaces. So I went a bit beyond the feedback, but I can easily undo that bit if it isn't needed :)
| @@ -63,6 +63,7 @@ export default makeMessages('feat.emails', { | |||
| noButtonText: m('Click to change this text!'), | |||
| }, | |||
| settings: { | |||
| defaultButtonTextWarning: m('This button still has the default text'), | |||
There was a problem hiding this comment.
Maybe we can change this to something that works regardless of what the error is? Something like "Make sure to add your text to the button" or something like that?
I trust you to come up with something reasonable. Just wanted to propose a bit of a shortcut here that might work. 😊
| @@ -33,6 +33,9 @@ const ButtonBlockListItem: FC<ButtonBlockLIstItemProps> = ({ | |||
| }, 400); | |||
|
|
|||
| const error = inputValue.length > 0 && !formatUrl(inputValue); | |||
| const buttonTextError = | |||
| !data.buttonText?.replaceAll(' ', '').trim().length || | |||
Description
This PR adds an error message to be displayed in case the user has not changed the default text of a button in an email draft.
Screenshots
Changes
Notes to reviewer
When playing around a bit, I found some possible areas for improvement.
I should be able to just add a second error message (along the lines of 'This button must have some text', 'This button cannot be empty), just let me know if I should file this as a new issue or simply build on the current issue and re-PR it.
Related issues
Resolves #2444