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

Moves <TextHighlight /> component to @wordpress/components package #18609

Merged
merged 12 commits into from
Dec 3, 2019

Conversation

getdave
Copy link
Contributor

@getdave getdave commented Nov 19, 2019

Closes #18074.

This PR extracts the TextHighlight used within LinkControl into a standalone component.

TextHighlight is a component which applies a <mark> tag around all/any matching strings within a string of text. This is useful for highlighting search terms within a string of text.

How has this been tested?

  • Manually verifying LinkControl hasn't broken and search terms are still highlighted.
  • Checking Storybook behaviour.

Screenshots

Types of changes

New feature (non-breaking change which adds functionality).

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR. .

@getdave getdave added [Feature] UI Components Impacts or related to the UI component system [Package] Block editor /packages/block-editor [Package] Components /packages/components labels Nov 19, 2019
@getdave getdave requested a review from talldan November 19, 2019 15:00
@getdave getdave marked this pull request as ready for review November 19, 2019 15:38
@getdave getdave added the Needs Technical Feedback Needs testing from a developer perspective. label Nov 19, 2019
@getdave getdave self-assigned this Nov 19, 2019
@gziolo gziolo requested a review from nerrad November 20, 2019 09:52
Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

I was wondering whether this component could be refactored to use this new experimental API just introduced by @nerrad in #17376:
__experimentalCreateInterpolateElement
#18623 illustrates how it can be used in action. In this case, it would be possible to avoid wrapping text with span elements.

packages/components/src/text-highlight/README.md Outdated Show resolved Hide resolved
packages/components/src/text-highlight/README.md Outdated Show resolved Hide resolved
packages/components/src/text-highlight/stories/index.js Outdated Show resolved Hide resolved
@getdave
Copy link
Contributor Author

getdave commented Nov 27, 2019

Thanks I'm going to get back to this just as soon as I can. Likely this Friday.

@getdave getdave changed the title Moves component to @wordpress/components package Moves TextHighlight component to @wordpress/components package Dec 3, 2019
@getdave getdave changed the title Moves TextHighlight component to @wordpress/components package Moves <TextHighlight /> component to @wordpress/components package Dec 3, 2019
@getdave getdave force-pushed the add/text-highlight-component branch from a8def98 to eeb48f2 Compare December 3, 2019 09:11
@getdave getdave requested a review from gziolo December 3, 2019 09:12
@getdave
Copy link
Contributor Author

getdave commented Dec 3, 2019

I was wondering whether this component could be refactored to use this new experimental API just introduced by @nerrad in #17376:
__experimentalCreateInterpolateElement
#18623 illustrates how it can be used in action. In this case, it would be possible to avoid wrapping text with span elements.

@gziolo I was intending to keep the <span> as it might be useful to be able to style things but I've realised that perhaps it's not necessary as CSS will let us style the default case and have overides for the highlight text anyway.

As a result, I've refactored to the new experimental API.

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

It looks like you need to regenerate one of the snapshots generated from Storybook story. Otherwise, Jest seems to be happy about the proposed refactor :)

Nice work, thanks for applying my feedback.

Feel free to merge once you get into agreement with Travis 😄

@getdave getdave merged commit 9b9250a into master Dec 3, 2019
@getdave getdave deleted the add/text-highlight-component branch December 3, 2019 14:30
@getdave
Copy link
Contributor Author

getdave commented Dec 3, 2019

Thanks for the reviews @gziolo! Much appreciated.

@youknowriad youknowriad added this to the Gutenberg 7.1 milestone Dec 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] UI Components Impacts or related to the UI component system Needs Technical Feedback Needs testing from a developer perspective. [Package] Block editor /packages/block-editor [Package] Components /packages/components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move TextHighlight component into @wordpress/components
3 participants