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

Replace TextControl with SearchControl in ContentSearch Component #280

Merged

Conversation

psorensen
Copy link
Contributor

@psorensen psorensen commented Jan 17, 2024

Description of the Change

How to test the Change

  1. Insert Content Picker or Post Picker block in /example
  2. Confirm search component is used and block is operating normally

Changelog Entry

  • Replaces TextControl with SearchControl in 'content-search' component
  • Adds Content Picker block to test and demonstrate content-picker component
  • Updates documentation to include new prop and updates demo gif

Credits

Props @psorensen

Checklist:

  • I agree to follow this project's Code of Conduct.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests pass.

@psorensen psorensen self-assigned this Jan 17, 2024
@psorensen psorensen marked this pull request as draft January 17, 2024 05:18
Copy link

github-actions bot commented Jan 17, 2024

Size Change: +161 B (0%)

Total Size: 65.4 kB

Filename Size Change
dist/index.js 65.4 kB +161 B (0%)

compressed-size-action

Copy link

cypress bot commented Jan 17, 2024

5 failed tests on run #692 ↗︎

5 1 0 0 Flakiness 0

Details:

git commit remove manual border
Project: 10up Block Components Commit: 81a8919dab
Status: Failed Duration: 01:23 💡
Started: Jan 18, 2024 7:34 PM Ended: Jan 18, 2024 7:36 PM
Failed  IconPicker.spec.js • 1 failed test

View Output Video

Test Artifacts
IconPicker > allows the user to use the post picker to change an icon and displays it Screenshots Video
Failed  Image.spec.js • 1 failed test

View Output Video

Test Artifacts
Image > allows the user to pick an image from the media library and displays it inline Screenshots Video
Failed  Link.spec.js • 1 failed test

View Output Video

Test Artifacts
Link > allows the editor to pick a link directly inline Screenshots Video
Failed  Repeater.spec.js • 1 failed test

View Output Video

Test Artifacts
Repeater > Adding Repeater field and saving it. Screenshots Video
Failed  registerBlockExtension.spec.js • 1 failed test

View Output Video

Test Artifacts
registerBlockExtension > ensure the new setting shows up and doesn't cause deprecation errors Screenshots Video

Review all test suite changes for PR #280 ↗︎

Copy link

🎉 A new testing version of this package has been published to NPM. You can install it with npm install @10up/block-components@testing-280

@psorensen psorensen changed the title replaces TextControl with SearchControl replace TextControl with SearchControl in content-search Jan 18, 2024
@psorensen psorensen marked this pull request as ready for review January 18, 2024 05:20
Copy link
Member

@Antonio-Laguna Antonio-Laguna left a comment

Choose a reason for hiding this comment

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

This LGTM! Thanks @psorensen !

Will let @fabiankaegy chime in since he might more opinion here!

Copy link
Member

@fabiankaegy fabiankaegy 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 making this update :)

I've left some suggestions for what I personally think looks better and feels more native in the editor.

Before After
CleanShot 2024-01-18 at 11 44 54@2x CleanShot 2024-01-18 at 11 44 26@2x

components/content-search/index.js Show resolved Hide resolved
components/content-search/index.js Outdated Show resolved Hide resolved
@@ -357,6 +359,8 @@ const ContentSearch = ({
onBlur={() => {
setIsFocused(false);
}}
size="compact"
style={{ border: '1px solid rgb(30, 30, 30)' }}
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure we need this manual border. With the greater space it has this should look better without I think

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My concern here is that I cannot see the border of the input at all - maybe it's my eyes, but I don't even realize it's a search input without the border

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah - there was something with my monitor - probably a high contrast setting (ironic enough) that was preventing me from seeing the gray background of the input. I've removed the manual style.

@fabiankaegy fabiankaegy changed the title replace TextControl with SearchControl in content-search Replace TextControl with SearchControl in ContentSearch Component Jan 18, 2024
@fabiankaegy fabiankaegy merged commit 1189822 into develop Jan 23, 2024
3 of 6 checks passed
@fabiankaegy fabiankaegy deleted the feature/content-search-with-searchcontrol-component branch January 23, 2024 15:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants