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

Fix sample code on InputControl docs #59517

Merged
merged 10 commits into from Mar 11, 2024

Conversation

flexseth
Copy link
Contributor

@flexseth flexseth commented Mar 1, 2024

What?

InputControl code samples broken - the useState package has moved from wordpress/compose to wordpress/element

How?

When trying to use an InputControl, I found that many of the Gutenberg package imports had been updated, but not those for the InputControl component. This affects the Developer Documentation and also Storybook.

There were three declarations that needed to be changed in the Gutenberg folder. Search & Replace in VS Code worked to fix the code and this PR represents the updated Gutenberg codebase with changes to the developer documentation.

Testing Instructions

  1. npx @wordpress/create-block input-control-block
  2. Start the build to watch for changes
  3. Activate the Input Control Block plugin
  4. Add an input control block to a new post
  5. Modify the edit function with the sample code, see steps below
  6. Add the import headers above the Edit() function
import { __experimentalInputControl as InputControl } from '@wordpress/components';
import { useState } from '@wordpress/compose';
  1. Change the Edit function to match sample code
export default function Edit() {
	const [ value, setValue ] = useState( '' );
	
	return (
		<InputControl
			value={ value }
			onChange={ ( nextValue ) => setValue( nextValue ?? '' ) }
		/>
	);
};
  1. At this point the block breaks
Screenshot 2024-03-01 at 5 10 47 PM console error shown below Screenshot 2024-03-01 at 5 11 16 PM

Testing the broken interface

  1. In Edit.js
    change
import { useState } from '@wordpress/compose';

to

import { useState } from '@wordpress/element';
  1. Save changes
  2. Refresh the page or post where you inserted the block
  3. The block should now show an input box
Screenshot 2024-03-01 at 5 25 18 PM

About this PR

This PR simply changes the location of the package imports, so no one else runs into this when trying to work with the InputControl component.

Screenshots included

  • UI when not working
  • Error code
  • Functional UI

@flexseth flexseth requested a review from ajitbohra as a code owner March 1, 2024 22:28
Copy link

github-actions bot commented Mar 1, 2024

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: flexseth <flexseth@git.wordpress.org>
Co-authored-by: t-hamano <wildworks@git.wordpress.org>
Co-authored-by: youknowriad <youknowriad@git.wordpress.org>
Co-authored-by: tyxla <tyxla@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@t-hamano t-hamano added the [Type] Developer Documentation Documentation for developers label Mar 2, 2024
@t-hamano t-hamano self-requested a review March 2, 2024 01:03
@t-hamano
Copy link
Contributor

t-hamano commented Mar 2, 2024

Thanks for the PR! Looking at #54908, it seems that it is currently recommended to use react instead of @wordpress/element 🤔

cc @youknowriad @tyxla

@flexseth
Copy link
Contributor Author

flexseth commented Mar 2, 2024

Thanks for the PR! Looking at #54908, it seems that it is currently recommended to use react instead of @wordpress/element 🤔

cc @youknowriad @tyxla

Thank you for bringing this up @t-hamano! I have seen the packages imported this way and never thought about the distinction until just now.

More info on the changes in React abstraction

Possible reason to use wordpress/element

There may be some additional controls offered for the InputControl component over the native React.js input component (see attributes), such as

  • prefix
  • suffix
  • _next40pxDefaultSize

Explanation of controls

  • set text to display before input field
  • set text to display after input field
  • make the input a little larger

Testing these controls with react import value

I will do some testing to see if I can use these controls without importing the package from Gutenberg.

Copy link
Contributor Author

@flexseth flexseth left a comment

Choose a reason for hiding this comment

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

Checked InputControl component with
import {useState} from 'react'

This abstraction works. There may be additional features of useState that are advanced and customized for the useState hook for WordPress. If this is the case, the block will break and the user will have to import the package directly from WordPress to use this functionality. This is a known limitation per

Note that we do have some functions in @wordpress/element that are additional like renderToString or createInterpolateElement...

@youknowriad
Copy link
Contributor

There's strictly no difference between @wordpress/element and react for these React APIs. I think we should just make sure that we're consistent in our code base.

It seems that right now:

  • For documentation purpose, we use react
  • For code, we're still using @wordpress/element

So let's just maintain that status-quo for now. which I guess mean ship this PR.

@@ -126,7 +126,7 @@ export function UnforwardedInputControl(
*
* ```jsx
* import { __experimentalInputControl as InputControl } from '@wordpress/components';
* import { useState } from '@wordpress/compose';
* import { useState } from '@wordpress/element';
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be "react" 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.

Changed import declaration to react.

This is sample code in the documentation, the code is wrapped in Markdown syntax representing JSX

```jsx

see notes on codebase consistency

Copy link
Contributor

Choose a reason for hiding this comment

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

It has already been mentioned, but isn't it "react" instead of "element"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will look into it. It's possible when I was making changes it wasn't syncing to the correct branch.

Still getting familiar with the workflow of Git and contributing upstream vs. local working copy.

Will report back, @t-hamano

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should be resolved now.

@t-hamano t-hamano added the [Package] Components /packages/components label Mar 4, 2024
Copy link
Member

@tyxla tyxla left a comment

Choose a reason for hiding this comment

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

It seems that right now:

For documentation purpose, we use react
For code, we're still using @wordpress/element

Riad put it really well. On top of that, other than another minor CHANGELOG correction, think this should be good to go - thanks!

packages/components/CHANGELOG.md Outdated Show resolved Hide resolved
@flexseth
Copy link
Contributor Author

flexseth commented Mar 4, 2024

Thank you @youknowriad, @t-hamano and @tyxla for support! I will update and squash the commits to clean this up. This has been a great exercise in understanding commit history and the upstream flow, so I really do appreciate your feedback and direction!

@flexseth
Copy link
Contributor Author

flexseth commented Mar 5, 2024

Thank you for the support on this @t-hamano.

Successfully rebased and updated refs/heads/docs/update-code-samples.

  • Commits rebased
  • Followed message standards for final commit
  • Some checks are not passing but are related to other packages - useEffect - mostly

Thanks again! This was by far the most challenging software thing I've ever wrestled with 😄

Requesting final review @tyxla - thanks for the help on the CHANGELOG update.

@flexseth flexseth requested a review from tyxla March 5, 2024 23:59
@flexseth
Copy link
Contributor Author

flexseth commented Mar 6, 2024

@tyxla - I tried to change the commit message and somehow added quite a few more commits. Please hold off on reviewing. I'll get some help from my mentor after they return from WC Asia.

@flexseth
Copy link
Contributor Author

flexseth commented Mar 6, 2024

/assign

@youknowriad
Copy link
Contributor

Don't worry too much about commit messages, the person merging the PR will change and adapt it properly.

@flexseth flexseth force-pushed the docs/update-code-samples branch 3 times, most recently from 1106940 to d03908b Compare March 10, 2024 20:49
@t-hamano t-hamano self-requested a review March 11, 2024 04:19
Copy link
Contributor

@t-hamano t-hamano left a comment

Choose a reason for hiding this comment

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

LGTM! I only deleted unnecessary spaces in CHANGELOG.

@t-hamano t-hamano merged commit 33056a4 into WordPress:trunk Mar 11, 2024
57 checks passed
@github-actions github-actions bot added this to the Gutenberg 18.0 milestone Mar 11, 2024
@flexseth
Copy link
Contributor Author

Thank you everyone for helping out on this!

carstingaxion pushed a commit to carstingaxion/gutenberg that referenced this pull request Mar 27, 2024
* Docs: Fixed sample code on InputControl docs

* Docs: Fixed sample code for InputControl

* Docs: Fixed sample code for InputControl

* Import useState from react package

* Use wordpress/element package on source code, React for documentation purposes.

* Using react in sample code (documentation)

* Remove unnecessary space

---------

Co-authored-by: flexseth <flexseth@git.wordpress.org>
Co-authored-by: t-hamano <wildworks@git.wordpress.org>
Co-authored-by: youknowriad <youknowriad@git.wordpress.org>
Co-authored-by: tyxla <tyxla@git.wordpress.org>
@flexseth flexseth deleted the docs/update-code-samples branch April 13, 2024 22:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Components /packages/components [Type] Developer Documentation Documentation for developers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants