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(rich-text-editor): Add tagLink, subscript, superscript, kbd, and spoiler input #158

Merged
merged 35 commits into from
Aug 4, 2022

Conversation

tmcentee
Copy link
Contributor

@tmcentee tmcentee commented Jul 7, 2022

Closes #51.

Describe your changes

This PR adds four new features to the Rich Text editor that were previously Markdown-only. All include keyboard shortcuts as well as manual entry via a new overflow dropdown menu option.

These include the following:

TagLinks CTRL-[

insert-tag

Subscript Ctrl - ; and Superscript Ctrl - :

subsup

Spoiler CTRL-]

spoiler

PR Checklist

  • All new/changed functionality includes unit and (optionally) e2e tests as appropriate
  • All new/changed functions have /** ... */ docs
  • I've added the bug/enhancement and other labels as appropriate

Environment(s) tested

  • Device: Desktop
  • OS: Windows
  • Browser: Chrome
  • Version: 103

@tmcentee tmcentee added the mode - rich text Affects the editor's rich text (wysiwyg) mode label Jul 7, 2022
@tmcentee tmcentee self-assigned this Jul 7, 2022
@netlify
Copy link

netlify bot commented Jul 7, 2022

Deploy Preview for nifty-lalande-39c157 ready!

Name Link
🔨 Latest commit e682c75
🔍 Latest deploy log https://app.netlify.com/sites/nifty-lalande-39c157/deploys/62e17e7569a216000808d54d
😎 Deploy Preview https://deploy-preview-158--nifty-lalande-39c157.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@tmcentee tmcentee added the enhancement New feature or request label Jul 8, 2022
@tmcentee tmcentee requested a review from b-kelly July 8, 2022 13:57
@tmcentee tmcentee marked this pull request as ready for review July 8, 2022 13:57
@tmcentee tmcentee changed the title Issue 51: Rich Text Input Parity for some Markdown-only nodes feat(rich-text-editor): Add tagLink, subscript, superscript, and spoiler input to rich-text editor Jul 12, 2022
@tmcentee tmcentee changed the title feat(rich-text-editor): Add tagLink, subscript, superscript, and spoiler input to rich-text editor feat(rich-text-editor): Add tagLink, subscript, superscript, and spoiler input Jul 12, 2022
@aalear
Copy link
Member

aalear commented Jul 13, 2022

The tag link shouldn't support multiple words. There's existing validation for allowed tag names in parse_tag_link() that could perhaps be reused?

@tmcentee tmcentee closed this Jul 13, 2022
@tmcentee tmcentee reopened this Jul 13, 2022
Tyler McEntee added 2 commits July 13, 2022 09:21
- util method to validate tagName content
- test update to cover disabled tag input
- renamed shortcut
@tmcentee
Copy link
Contributor Author

@aalear Thanks for the heads-up. I pushed some changes to better validate the links. Now you can only convert the selection if the trimmed content doesn't have any whitespace. (trimmed because double clicking words in Chrome on Windows also selects the space after the word)

@b-kelly b-kelly added the needs design Needs a design pass to clean up the UI or UX label Jul 13, 2022
Copy link
Collaborator

@b-kelly b-kelly left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I have added some suggestions for changes inline

test/rich-text/commands/index.test.ts Outdated Show resolved Hide resolved
src/rich-text/commands/index.ts Outdated Show resolved Hide resolved
src/rich-text/commands/index.ts Show resolved Hide resolved
src/rich-text/commands/index.ts Show resolved Hide resolved
src/rich-text/commands/index.ts Show resolved Hide resolved
src/rich-text/commands/index.ts Outdated Show resolved Hide resolved
src/rich-text/commands/index.ts Outdated Show resolved Hide resolved
src/rich-text/commands/index.ts Outdated Show resolved Hide resolved
test/rich-text/commands/index.test.ts Show resolved Hide resolved
@tmcentee tmcentee requested a review from b-kelly July 15, 2022 16:19
@tmcentee
Copy link
Contributor Author

@b-kelly I think I've addressed everything save for my comment about toggleWrapIn versus replace text for the tagLink node.

I also added <kbd> mark type and test updates.

@tmcentee tmcentee changed the title feat(rich-text-editor): Add tagLink, subscript, superscript, and spoiler input feat(rich-text-editor): Add tagLink, subscript, superscript, kbd, and spoiler input Jul 18, 2022
@tmcentee
Copy link
Contributor Author

I pushed updates to the keyboard shortcuts for superscript and subscript to match most other editor conventions.

Copy link
Collaborator

@b-kelly b-kelly left a comment

Choose a reason for hiding this comment

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

Note - blocked by #157, which also exposes toggleWrapIn, adding tests and docs

src/shared/utils.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@b-kelly b-kelly left a comment

Choose a reason for hiding this comment

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

Imo, the rich-text version is ready for merge. Any chance we can get the dropdown menu in markdown mode as well? I'd like to keep the menus as aligned as possible. If this is an outsized request, we can consider getting this PR merged first.

@@ -70,6 +71,19 @@ export function allKeymaps(
...bindLetterKeymap("Mod-h", toggleHeadingLevel()),
...bindLetterKeymap("Mod-r", insertHorizontalRuleCommand),
...bindLetterKeymap("Mod-m", setBlockType(schema.nodes.code_block)),
...bindLetterKeymap(
"Mod-[",
Copy link
Collaborator

Choose a reason for hiding this comment

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

reviewer's note: mod+[/] are commonly used in IDEs to (un)indent a line. This shouldn't pose too much of an issue though, as we can chain commands together, placing the code indent command first, falling back to creating a taglink.

No action necessary.

@b-kelly b-kelly force-pushed the tmcentee/editor-51/new-rich-text-nodes branch from 124e993 to e49a2f8 Compare July 25, 2022 17:42
@tmcentee
Copy link
Contributor Author

tmcentee commented Jul 26, 2022

@b-kelly

If this is an outsized request, we can consider getting this PR merged first.

I think we should get this merged and follow up with the Markdown menu items. I don't have a ton of time this sprint to dig into it myself.

@b-kelly b-kelly merged commit 77fa060 into main Aug 4, 2022
@b-kelly b-kelly deleted the tmcentee/editor-51/new-rich-text-nodes branch August 4, 2022 14:18
b-kelly pushed a commit that referenced this pull request Aug 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request mode - rich text Affects the editor's rich text (wysiwyg) mode needs design Needs a design pass to clean up the UI or UX
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add rich text insert equivalents of all markdown-only nodes
4 participants