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

Edit Post: Wrap PluginPostStatusInfo with PanelRow rather than Slot #6867

Merged
merged 2 commits into from Jun 8, 2018

Conversation

@gziolo
Member

gziolo commented May 21, 2018

Description

Closes #6839 raised by @ryanboswell.
Improves the implementation from #6300 where PanelRow was wrongly applied to the Slot where it should be done for Fill.

Before

<div class="components-panel__row">
    <div>
        <button type="button" class="components-button button-link ">Here is a Button</button>
    </div>
</div>

After

<div>
    <div class="components-panel__row my-custom-class-name">
        <button type="button" class="components-button button-link ">Here is a Button</button>
    </div>
</div>

How has this been tested?

Manually with the following JS code pasted into JS console:

I tested with the following code:

var el = wp.element.createElement;
var __ = wp.i18n.__;
var registerPlugin = wp.plugins.registerPlugin;
var PluginPostStatusInfo = wp.editPost.PluginPostStatusInfo;

function MyPlugin() {
	return (
		el(
			PluginPostStatusInfo,
			{ className: 'my-custom-class-name' },
			__( 'My post status info' )
		)
	);
};

registerPlugin( 'my-plugin', {
	render: MyPlugin,
} );

Also with unit tests: npm test.

Types of changes

Bug fix (non-breaking change which fixes an issue).

Checklist:

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

@gziolo gziolo self-assigned this May 21, 2018

@gziolo gziolo requested review from adamsilverstein and WordPress/gutenberg-core May 21, 2018

@gziolo

This comment has been minimized.

Member

gziolo commented May 21, 2018

@gziolo gziolo added this to the 3.0 milestone May 21, 2018

@ryanboswell

This comment has been minimized.

ryanboswell commented May 21, 2018

Hmm, this might improve some of the semantics and make sure that PanelRow is wrapped around whatever children are being added (so developers don't need to themselves I guess), but it doesn't solve the original problem with the unstyleable empty div. The results I get visually end up like this:

button-with-patch

Versus this is the same button before the patch:

button-without-patch

Having .components-panel__row applied to the top-most element seems to be the way to go to make sure the row overall gets positioned properly relative to the others, moving it around doesn't appear to help that there's a <div> in between that resets any positioning or flexbox alignments.

@ryanwelcher

This comment has been minimized.

Contributor

ryanwelcher commented May 23, 2018

The issue seems to be that no matter where the PanelRow is added, the Slot always outputs an empty div that wraps any children. I have a PR ( #6905 ) against this branch that addresses this by allowing the div in the Slot to have a className passed to it via props.

@ryanwelcher

This comment has been minimized.

Contributor

ryanwelcher commented May 23, 2018

@gziolo

This comment has been minimized.

Member

gziolo commented May 23, 2018

I will check if we can get rid of this wrapping element here and in other similar cases.

@adamsilverstein

This comment has been minimized.

Contributor

adamsilverstein commented May 23, 2018

@ryanwelcher what about your idea to optionally pass a Component to createSlotFill and have the Slot use that instead of the div?

@ryanwelcher

This comment has been minimized.

Contributor

ryanwelcher commented May 23, 2018

@adamsilverstein the PR for that is #6920

@youknowriad youknowriad modified the milestones: 3.0, 3.1 May 30, 2018

@gziolo gziolo added this to In progress in Extensibility Jun 7, 2018

@gziolo

This comment has been minimized.

Member

gziolo commented Jun 7, 2018

This PR won't fix #6839 but fixes an existing bug so I would prefer to land it anyway. Let's discuss in #6920 how to attack the original issue.

@gziolo gziolo changed the title from Edit Post: Avoid unstylable <div> elements around fills to Edit Post: Wrap PluginPostStatusInfo with PanelRow rather than Slot Jun 7, 2018

@jorgefilipecosta

This comment has been minimized.

Member

jorgefilipecosta commented Jun 7, 2018

I noticed the same visual regression poited out by @ryanboswell:
screen shot 2018-06-07 at 22 12 59

That happens because we have different styles on the first-panel row and the other panel rows given that we have a wrapper div here, the styles for first-panel row are wrongly applied. This problem gets solved if the wrapper is removed.

@gziolo

This comment has been minimized.

Member

gziolo commented Jun 7, 2018

@jorgefilipecosta, the thing is that we shouldn’t be removing those div wrappers. I have an idea, we can render Slot inside PostStatus component using render props for Fills 😃

@jorgefilipecosta

jorgefilipecosta approved these changes Jun 7, 2018 edited

The code changes look right to me, and this PR corrects the problem of all fills being in the same rows:
Before:
screen shot 2018-06-07 at 22 19 08
After:
screen shot 2018-06-07 at 22 19 38

That said there is a visual problem and that I think should be addressed in the same release as this PR. The optimal way to solve would be to remove the wrapper div, but given the form, slot fill works that may not be possible. Unless we make some changes like renaming the slot to PostStatusInfo and the core components rendered in the same area also use the slot so the wrapper div includes the core components and we don't have this styles problem, but may not be easy to guarantee trash appears as the last fill.

Another way to solve it is we can apply some CSS rule to correct the styles while we don't have a better fix.

These changes seem required in all the paths we can choose.

@gziolo

This comment has been minimized.

Member

gziolo commented Jun 8, 2018

This is what I got after pushing f21b090:

Desktop:
screen shot 2018-06-08 at 06 44 33

Mobile:
screen shot 2018-06-08 at 06 44 56

@gziolo

This comment has been minimized.

Member

gziolo commented Jun 8, 2018

@ryanboswell - I might have fix your original issue, can you confirm?

@gziolo gziolo merged commit a60b0f2 into master Jun 8, 2018

2 checks passed

codecov/project Absolute coverage decreased by -0.12% but relative coverage increased by +20.04% compared to 102445b
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

Extensibility automation moved this from In progress to Done Jun 8, 2018

<PostAuthor />
<PluginPostStatusInfo.Slot />
<PostTrash />
<PluginPostStatusInfo.Slot>

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Jun 8, 2018

Member

It may make sense to rename the slot fill to PostStatusInfo as the slot is not only used to Plugins.

This comment has been minimized.

@gziolo

gziolo Jun 8, 2018

Member

Yes, we discussed that before with @mtias, but decided to keep the prefix Plugin to better express the intent. The idea is to use those fills only with registerPlugin API.

@gziolo gziolo deleted the fix/6839-post-status-info branch Aug 30, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment