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 "display post date" and "number of posts to show" Inspector options for Latest Posts block #1191

Merged
merged 13 commits into from
Jun 29, 2017

Conversation

lamosty
Copy link
Member

@lamosty lamosty commented Jun 15, 2017

When building the Latest Posts block, @jasmussen did a great design of some initial Inspector options. In this PR, we are implementing parts of it.

Testing

  1. In a post, add the latest posts block and click on it;
  2. On the left of the screen, the Gutenberg Inspector should show the "display post date" and "number of posts to show" options;
  3. Play with the options -- do they work as expected?

Looking like this as of now:

selection_359

TODO

  • Add display post date to server-side rendering
  • Possibly debounce the change in number of posts to show input

@lamosty lamosty added [Feature] Blocks Overall functionality of blocks [Status] In Progress Tracking issues with work in progress labels Jun 15, 2017
@lamosty lamosty self-assigned this Jun 15, 2017
@westonruter
Copy link
Member

@lamosty I was doing this as well in #1183.

@lamosty lamosty force-pushed the add/latest-posts-inspector branch 2 times, most recently from ad79c59 to a0a4a34 Compare June 15, 2017 13:56
@lamosty lamosty requested a review from youknowriad June 15, 2017 15:02
@westonruter westonruter force-pushed the add/latest-posts-inspector branch from 13bcbf6 to 211981a Compare June 19, 2017 04:17
@westonruter
Copy link
Member

Rebased to remove merge commit 13bcbf6 (and former HEAD).

className="editor-latest-posts__input"
onChange={ () => this.changePostsToShow( this.refs[ postToShowId ].value ) }
/>
</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we wait for this #1202 and use the common controls declared there?

Copy link
Contributor

Choose a reason for hiding this comment

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

We should absolutely use the same controls everywhere, but whether this happens post merge or not I don't think is important. Should I open a separate ticket for making sure controls are all the same?

Copy link
Contributor

Choose a reason for hiding this comment

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

The controls are merged, let's rebase this :)

@westonruter
Copy link
Member

So now this is waiting on #1278?

@jasmussen
Copy link
Contributor

So now this is waiting on #1278?

I don't think it has to. IMO okay to merge so long as we remember to address issues after the fact.

@jasmussen
Copy link
Contributor

Took the liberty of pushing a tiny style tweak to the post date:

screen shot 2017-06-23 at 11 57 01

let { poststoshow: postToShowNext } = nextProps.attributes;
const { setAttributes } = this.props;

postToShowNext = parseInt( postToShowNext );
Copy link
Contributor

Choose a reason for hiding this comment

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

The parseInt is a bit weird, we should get the JSON attributes PR merged soon :)

Copy link
Member

Choose a reason for hiding this comment

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

Please always add the radix. :)

Copy link
Contributor

@youknowriad youknowriad Jun 28, 2017

Choose a reason for hiding this comment

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

I think we should probably drop it since the JSON attributes are merged.

className="editor-latest-posts__input"
onChange={ () => this.changePostsToShow( this.refs[ postToShowId ].value ) }
/>
</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

The controls are merged, let's rebase this :)

},

edit: class extends wp.element.Component {
edit: withInstanceId( class extends wp.element.Component {
Copy link
Contributor

Choose a reason for hiding this comment

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

We're moving way from wp.* as much as we can in favor of import { Component } from 'element';

@ellatrix
Copy link
Member

Rebasing...

@ellatrix ellatrix force-pushed the add/latest-posts-inspector branch from 10eb4b4 to ec84d3e Compare June 29, 2017 08:53
@ellatrix ellatrix requested a review from youknowriad June 29, 2017 09:37
@ellatrix
Copy link
Member

What else is left here?

@jasmussen
Copy link
Contributor

I think it's good to go, probably!

@ellatrix
Copy link
Member

@jasmussen Fixed padding issue that I missed. Should be good to go for now.

There's a rule that's overly broad in Gutenberg:

.gutenberg ul,
.gutenberg ol {
  margin: 0;
  padding: 0;
}

I'm not sure why this is done, because now blocks are forced to overwrite it by adding a parent selector.

@ellatrix
Copy link
Member

Also adjusted the value: padding-left: 2.5em;. This should probably be set by the editor, not the block, so it is inherited by both the list block and the latest post block.

@jasmussen
Copy link
Contributor

You're right, we should probably scope that UL code a bit. I can't recall the circumstances right now, but I think it's just basic CSS normalization.

@ellatrix
Copy link
Member

Okay, let's merge and iterate.

@ellatrix ellatrix merged commit e37894c into master Jun 29, 2017
@ellatrix ellatrix deleted the add/latest-posts-inspector branch June 29, 2017 10:17
@lamosty
Copy link
Member Author

lamosty commented Jun 29, 2017

Thanks for finishing this, I couldn't find time for it, sorry!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Blocks Overall functionality of blocks [Status] In Progress Tracking issues with work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants