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

Add props to withFilters #10195

Closed
wants to merge 5 commits into from

Conversation

adamsilverstein
Copy link
Member

Description

Another potential solution to #7020 this PR enhances the withFilters higher order component to also run the components props thru a filter. This enables filtering of the props passed to a filtered component and in the case of the publish buttons would allow developers to filter the isSavable prop. As with other filters applied with withFilters the component receives a forced update when a filter is added or removed.

How has this been tested?

I didn't get very far with testing yet, this is a POC so far.

Screenshots

Types of changes

  • Filter props in withFilters using a new filter that is 'FilterName' + 'Props' (filtername is passed to the function)
  • filter the publish toggle button and post publish panel

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

@adamsilverstein adamsilverstein changed the title Add props to withfilters Add props to withFilters Sep 26, 2018
@@ -28,6 +28,7 @@ export default function withFilters( hookName ) {
/** @inheritdoc */
constructor( props ) {
super( props );
this.ComponentProps = applyFilters( hookName + 'Props', props );
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be called also inside throttledForceUpdate

Copy link
Member Author

Choose a reason for hiding this comment

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

good point - thanks for the feedback - addressed in 08f27c3. If you think this might be something we would adopt, I can take a pass at adding some tests.

@adamsilverstein
Copy link
Member Author

Not sure why the test is failing? Any other feedback/ideas for this PR?

@aduth
Copy link
Member

aduth commented Oct 10, 2018

Between this and #10115, I'm more in favor of #10115 since it reflects this as a condition on the state of the editor, not on its UI, which is merely an artifact. For example, nothing otherwise stops a save from occurring (e.g. maybe autosave still happening?)

@gziolo
Copy link
Member

gziolo commented Oct 15, 2018

I still this PR its valuable on its own to make withFilters more useful, but I have another idea how to solve the original issue. I will comment on #10115.

@gziolo gziolo added this to the 4.2 - API freeze milestone Oct 15, 2018
@gziolo gziolo added [Type] Technical Prototype Offers a technical exploration into an idea as an example of what's possible [Feature] Extensibility The ability to extend blocks or the editing experience labels Oct 15, 2018
@youknowriad
Copy link
Contributor

What's the status for this PR. Can it be ready for 4.2?

@gziolo
Copy link
Member

gziolo commented Oct 27, 2018

I will update it and land on Monday. We no longer need to apply changes to publish components but having a way to filter props might simplify a few things.

@youknowriad youknowriad modified the milestones: 4.2, WordPress 5.0 Oct 30, 2018
@gziolo
Copy link
Member

gziolo commented Oct 30, 2018

I will update it and land on Monday.

I hope to get back to it tomorrow. 4.2-RC1 is out so I should have some time to take care of it.

@adamsilverstein
Copy link
Member Author

@gziolo let me know if you need any testing on your updated approach!

@gziolo
Copy link
Member

gziolo commented Oct 31, 2018

I'm coming to the conclusion that we could achieve a similar result by using the existing logic as follows:

import { withFilters } from '@wordpress/components';
import { addFilter } from '@wordpress/hooks';

const MyComponent = ( { hint, title } ) => (
    <div>
        <h1>{ title }</h1>
        <p>{ hint }</p>
    </div> 
);

const MyComponentWithFilters = withFilters( 'MyHookName' )( MyComponent );

addFilter(
    'MyHookName',
    'example/filtered-component',
    ( FilteredComponent ) => ( props ) => (
        <FilteredComponent
            { ...props }
            hint="Overriden hint"
        />
    )
);

This way you wouldn't have to introduce a new filter but still, you would be able to override one of the existing props. Let me know if that makes sense.

@adamsilverstein
Copy link
Member Author

@gziolo That does make sense, although it took a while to sink in what the code does.

Seems like it would be worth documenting this approach if it isn;t already, at the very least adding your example so developers know how that can filter props.

@gziolo
Copy link
Member

gziolo commented Oct 31, 2018

Docs updated in #11313. Let's continue there. I'm closing this one. Thanks @adamsilverstein for starting this discussion. I didn't come up with this idea earlier, but now it should be more clear with the proposed changes to the documentation.

@gziolo gziolo closed this Oct 31, 2018
@gziolo gziolo deleted the feature/add-props-to-withfilters branch October 31, 2018 15:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Extensibility The ability to extend blocks or the editing experience [Type] Technical Prototype Offers a technical exploration into an idea as an example of what's possible
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants