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: support CodeBlock #3790

Merged
merged 22 commits into from
Aug 10, 2023
Merged

feat: support CodeBlock #3790

merged 22 commits into from
Aug 10, 2023

Conversation

TomasSlama
Copy link
Contributor

@TomasSlama TomasSlama commented Aug 9, 2023

FX-4184

Description

The result of merging all separate PRs

How to test

Screenshots

image

Development checks

Breaking change

  • codemod is created and showcased in the changeset
  • test alpha package of Picasso in StaffPortal

All development checks should be done and set checked to pass the
GitHub Bot: TODOLess action

PR commands

List of available commands:

  • @toptal-bot run package:alpha-release - Release alpha version
  • @toptal-anvil ping reviewers - Ping FX team for review
PR Review Guidelines

When to approve? ✅

You are OK with merging this PR and

  1. You have no extra requests.
  2. You have optional requests.
    1. Add nit: to your comment. (ex. nit: I'd rename this variable from makeCircle to getCircle)

When to request changes? ❌

You are not OK with merging this PR because

  1. Something is broken after the changes.
  2. Acceptance criteria is not reached.
  3. Code is dirty.

When to comment (neither ✅ nor ❌)

You want your comments to be addressed before merging this PR in cases like:

  1. There are leftovers like unnecessary logs, comments, etc.
  2. You have an opinionated comment regarding the code that requires a discussion.
  3. You have questions.

How to handle the comments?

  1. An owner of a comment is the only one who can resolve it.
  2. An owner of a comment must resolve it when it's addressed.
  3. A PR owner must reply with ✅ when a comment is addressed.

@TomasSlama TomasSlama added the Feature New feature or request label Aug 9, 2023
@TomasSlama TomasSlama self-assigned this Aug 9, 2023
@changeset-bot
Copy link

changeset-bot bot commented Aug 9, 2023

🦋 Changeset detected

Latest commit: 8fdc11f

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@toptal/picasso-rich-text-editor Major
@toptal/picasso-forms Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@TomasSlama TomasSlama changed the title Feature/code block feat: support CodeBlock Aug 9, 2023
@toptal-devbot toptal-devbot temporarily deployed to staging August 9, 2023 13:15 Inactive
@TomasSlama TomasSlama marked this pull request as ready for review August 9, 2023 13:18
@TomasSlama TomasSlama requested review from a team, mkrl and sashuk August 9, 2023 13:18
@TomasSlama
Copy link
Contributor Author

@toptal-anvil ping reviewers

@toptal-devbot toptal-devbot temporarily deployed to staging August 9, 2023 13:29 Inactive
@toptal-devbot toptal-devbot temporarily deployed to staging August 10, 2023 07:35 Inactive
@TomasSlama
Copy link
Contributor Author

@toptal-anvil ping reviewers

@sashuk
Copy link
Contributor

sashuk commented Aug 10, 2023

Do you think the output could be cleaned up a bit so it does not have those extra <span/>? Like in

Screenshot 2023-08-10 at 10 28 03

NIT: What do you think about adding a code black example to story (although it is already tracked in visual tests for RichText)?

Screenshot 2023-08-10 at 10 30 35

Copy link
Contributor

@sashuk sashuk left a comment

Choose a reason for hiding this comment

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

Tested locally (temploy returns 500 error):

  • copying and pasting from RichText line-by-line and as a whole
  • toggling formatting for code blocks
  • copying and pasting code blocks into each other, copying between editors, copying and pasting other types of rich text into code blocks and vice-versa

@mkrl
Copy link
Contributor

mkrl commented Aug 10, 2023

Tested locally.

When only the empty lines are present in the editor, it lacks the margin that is there otherwise.

deepin-screen-recorder_Select.area_20230810122849.mp4

Also there's one thing that I'm not sure we should fix, but you can't really select existing part of the text and turn it into code block, it creates a block up until the existing node / paragraph boundary

deepin-screen-recorder_Select.area_20230810123256.mp4

@TomasSlama
Copy link
Contributor Author

@mkrl The second issue is the same as we had with headings, it is still one paragraph as there is just br as space between.

I was actually trying even to convert just selected text, but it is pretty complex with a lot of edge cases. There is no specific requirement for this behaviour and I realized that there is not much benefit and it would be inconsistent with headings

@toptal-devbot toptal-devbot temporarily deployed to staging August 10, 2023 11:13 Inactive
@TomasSlama
Copy link
Contributor Author

@toptal-anvil ping reviewers

@TomasSlama
Copy link
Contributor Author

thank you guys for finding these bugs 🙇, I addressed them all

@toptal-devbot toptal-devbot temporarily deployed to staging August 10, 2023 11:29 Inactive
Copy link
Contributor

@sashuk sashuk left a comment

Choose a reason for hiding this comment

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

Comments were addressed, thank you Tomas for such enormous work!

Copy link
Contributor

@mkrl mkrl left a comment

Choose a reason for hiding this comment

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

Tremendous work, thank you!

@TomasSlama TomasSlama merged commit 4d0ec2b into master Aug 10, 2023
12 checks passed
@TomasSlama TomasSlama deleted the feature/code-block branch August 10, 2023 12:27
@toptal-build toptal-build mentioned this pull request Aug 10, 2023
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants