Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(RichTextEditor): enhanced look, link/quote/code block buttons [WPB-15264] #18643

Merged
merged 43 commits into from
Jan 24, 2025

Conversation

olafsulich
Copy link
Contributor

@olafsulich olafsulich commented Jan 22, 2025

TaskWPB-15264 Enhance Rich Text Editor - PRD + Bugs

Description

What's changed:

  • inserting links/blockquotes/code blocks via format buttons
  • format buttons new layout and styles
  • aligned editing message colors to the mobile app (using accent color)

Adding link:

adding-link.mov

Editing link:

editing-link.mov

Auto link:

auto-link.mov

Link validation:

link-validation.mov

Inserting blockquote and code blocks:

blockquote-code-block.mov

Theme changes & edit a message:

theme.mov

Checklist

  • mentions the JIRA issue in the PR name (Ex. [WPB-XXXX])
  • PR has been self reviewed by the author;
  • Hard-to-understand areas of the code have been commented;
  • If it is a core feature, unit tests have been added;

@olafsulich olafsulich requested review from otto-the-bot and a team as code owners January 23, 2025 06:15
@codecov-commenter
Copy link

codecov-commenter commented Jan 23, 2025

Codecov Report

Attention: Patch coverage is 20.43796% with 218 lines in your changes missing coverage. Please review.

Project coverage is 45.96%. Comparing base (f465eda) to head (9b87da2).
Report is 3 commits behind head on dev.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev   #18643      +/-   ##
==========================================
- Coverage   46.22%   45.96%   -0.26%     
==========================================
  Files         880      894      +14     
  Lines       27109    27347     +238     
  Branches     6118     6170      +52     
==========================================
+ Hits        12532    12571      +39     
- Misses      13017    13213     +196     
- Partials     1560     1563       +3     

Copy link
Contributor

@e-maad e-maad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍
Added few suggestion.

})}
type="button"
onClick={onClick}
title={isActive ? t('tooltipConversationHideFormatting') : t('tooltipConversationShowFormatting')}
aria-label={isActive ? t('tooltipConversationHideFormatting') : t('tooltipConversationShowFormatting')}
data-uie-name="format-text"
>
<Icon.MarkdownIcon />
<Icon.MarkdownIcon width={14} height={14} />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we make the icon size responsive using a relative unit (rem)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really, I don't see a reason to do that

</div>
<form css={formStyles} onSubmit={handleSubmit} noValidate>
<Input
ref={textInputRef}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to use callback here to avoid timeout in in useEffect?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so, the timeout is actually needed there, I added a comment next to it why it's there

value={formData.text}
markInvalid={isSubmitted && !!errors.text}
onChange={event => handleInputChange({event, field: 'text'})}
error={!!isSubmitted && !!errors.text ? <ErrorMessage>{errors.text}</ErrorMessage> : undefined}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this possible?

Suggested change
error={!!isSubmitted && !!errors.text ? <ErrorMessage>{errors.text}</ErrorMessage> : undefined}
error={!!isSubmitted && !!errors.text && <ErrorMessage>{errors.text}</ErrorMessage>}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would need to refactor it somehow because it's throwing an error:

Type 'false | Element' is not assignable to type 'ReactElement<any, string | JSXElementConstructor<any>> | undefined'.
  Type 'boolean' is not assignable to type 'ReactElement<any, string | JSXElementConstructor<any>>'.

It's reasonable, when !!isSubmitted && !!errors.url will be false:

  • When isSubmitted is false
  • When errors.url is undefined/null/empty

@e-maad e-maad self-requested a review January 24, 2025 09:15
Copy link
Contributor

@e-maad e-maad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🚀

@olafsulich olafsulich merged commit e714abb into dev Jan 24, 2025
14 of 16 checks passed
@olafsulich olafsulich deleted the feat/rich-text-new-look branch January 24, 2025 09:20
@paulwire paulwire added the echoes: product-roadmap/feature Work contributing to adding a new feature as part of the product roadmap. label Jan 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
echoes: product-roadmap/feature Work contributing to adding a new feature as part of the product roadmap. 👕 size: XXL type: feature / request ✨
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants