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.

Sign up for GitHub

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

Jump to bottom

[EuiMarkdownEditor] Highlight relevant toolbar button when in a matching text node #5840

Merged
merged 9 commits into from
Apr 28, 2022
Merged

[EuiMarkdownEditor] Highlight relevant toolbar button when in a matching text node #5840

merged 9 commits into from
Apr 28, 2022

Conversation

chandlerprall
Copy link
Contributor

@chandlerprall chandlerprall commented Apr 25, 2022 •
edited
Loading

Summary

Opening as a draft to ensure CI is happy before reviews

  • properly linked the toolbar buttons with their markdown AST nodes so the buttons will highlight as the user navigates the markdown
  • added aria-pressed to the toolbar buttons
  • created an initial markdown cypress spec, added test cases for the button<->node linkage

I wanted to take this further and make the buttons highlight for nested nodes (e.g. italic in bold), but the button actions don't function in that environment so I think it's better to avoid handling element nesting when determining the button state.

Before

markdown

After

markdown-after

Checklist

- [ ] Checked in both light and dark modes

  • Checked in mobile
  • Checked in Chrome, Safari, Edge, and Firefox
    - [ ] Props have proper autodocs and playground toggles
    - [ ] Added documentation
    - [ ] Checked Code Sandbox works for any docs examples
  • Added or updated jest and cypress tests
  • Checked for breaking changes and labeled appropriately
  • Checked for accessibility including keyboard-only and screenreader modes
    - [ ] Updated the Figma library counterpart
  • A changelog entry exists and is marked appropriately

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5840/

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5840/

@chandlerprall chandlerprall changed the title Highlight relevant toolbar button when in a matching text node [EuiMarkdownEditor] Highlight relevant toolbar button when in a matching text node Apr 25, 2022
@chandlerprall chandlerprall marked this pull request as ready for review April 25, 2022 20:39
@chandlerprall
Copy link
Contributor Author

@1Copenut I requested your review specifically for the added aria-pressed attribute, no hurry on taking a look

@cchaos cchaos requested a review from elizabetdev April 25, 2022 20:55
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5840/

@chandlerprall chandlerprall requested a review from cee-chen April 27, 2022 17:26
@thompsongl
Copy link
Contributor

jenkins test this

@cee-chen
Copy link
Contributor

Super brief QA testing notes:

  1. it looks like stacking bold and italics doesn't highlight both - not sure if that matters? EDIT: just saw your PR description sorry, that totally makes sense re: behavior. I'm good with it

  2. Quotes only appear to highlight when the first word of the quote, and not when clicking anywhere in the quote without selection
    screencap

  3. Not a huge deal, but link highlighting does not work unless you manually type a valid prefix, e.g. http://url. Should we consider changing the default url placeholder that appears when you click the link button, so that it registers as a link on insertion?

  4. All of the list item buttons don't appear to have working highlighting - is this intentional? (no worries if so, just checking)

cee-chen
cee-chen reviewed Apr 27, 2022
View reviewed changes
Copy link
Contributor

@cee-chen cee-chen left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Code looks great to me with very minor/optional comments! My only remaining comments are on the QA items above, particularly the quote button behavior

src/components/markdown_editor/markdown_editor.spec.tsx Outdated Show resolved Hide resolved
src/components/markdown_editor/markdown_editor_toolbar.tsx Outdated Show resolved Hide resolved
1Copenut
1Copenut suggested changes Apr 27, 2022
View reviewed changes
Copy link
Contributor

@1Copenut 1Copenut left a comment

Choose a reason for hiding this comment

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

I really wanted to keep the aria-pressed attribute, but came down on the side of removing it. Reasons and documentation linked in the review notes.

src/components/markdown_editor/markdown_editor_toolbar.tsx Outdated Show resolved Hide resolved
@cee-chen
Copy link
Contributor

jenkins test this

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5840/

elizabetdev
elizabetdev suggested changes Apr 28, 2022
View reviewed changes
Copy link
Contributor

@elizabetdev elizabetdev left a comment

Choose a reason for hiding this comment

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

Hi @chandlerprall,

I tested the PR and found a bug. When we click on some of the toolbar plugins the following error appears:

Screen.Recording.2022-04-28.at.04.28.02.PM.mp4

@chandlerprall
Copy link
Contributor Author

@constancecchen

Quotes only appear to highlight when the first word of the quote, and not when clicking anywhere in the quote without selection

This happens because the quote node's content is a paragraph, and without addressing nested nodes it can't detect it is in a quote. That said, I've removed the quote button highlighting as clicking it when highlighted didn't toggle the quote syntax away, but instead inserted a second quote.

Not a huge deal, but link highlighting does not work unless you manually type a valid prefix, e.g. http://url. Should we consider changing the default url placeholder that appears when you click the link button, so that it registers as a link on insertion?

Done! Updated the default url to https:// which triggers the button association.

All of the list item buttons don't appear to have working highlighting - is this intentional? (no worries if so, just checking)

Intentional, same reason that I gave for disabling the quote button.

@chandlerprall
Copy link
Contributor Author

@miukimiu pushed a fix for that bug

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5840/

1Copenut
1Copenut approved these changes Apr 28, 2022
View reviewed changes
Copy link
Contributor

@1Copenut 1Copenut left a comment

Choose a reason for hiding this comment

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

LGTM!

cee-chen
cee-chen approved these changes Apr 28, 2022
View reviewed changes
Copy link
Contributor

@cee-chen cee-chen left a comment

Choose a reason for hiding this comment

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

Fixes look great!

elizabetdev
elizabetdev approved these changes Apr 28, 2022
View reviewed changes
Copy link
Contributor

@elizabetdev elizabetdev left a comment

Choose a reason for hiding this comment

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

Thanks, @chandlerprall. Tested again and the error is gone. LGTM! 🎉

@chandlerprall chandlerprall merged commit 08c2c3f into elastic:main Apr 28, 2022
@chandlerprall chandlerprall deleted the markdown/toolbar-button-states branch April 28, 2022 19:30
@breehall breehall mentioned this pull request May 16, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cee-chen cee-chen cee-chen approved these changes

@1Copenut 1Copenut 1Copenut approved these changes

@elizabetdev elizabetdev elizabetdev approved these changes

Assignees
No one assigned
Labels
markdown editor
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@chandlerprall @kibanamachine @thompsongl @cee-chen @1Copenut @elizabetdev