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

[WIP] Add alignment for latest posts block #1183

Closed
wants to merge 9 commits into from

Conversation

westonruter
Copy link
Member

This is to implement part of #1011.

This is my very first hacking with blocks, so I'm sure I'm probably doing some things wrong. Advice appreciated.

The alignment buttons should get moved into a separate component that can be re-used across all widgets.

Also need to implement support for align in the server-side rendering.

</ul>
</div>
}
{ /* focus && */
Copy link
Member Author

Choose a reason for hiding this comment

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

When I uncomment this, the controls don't show up at all. Not sure why.

Copy link
Contributor

Choose a reason for hiding this comment

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

I suspect that it's because you're saving the focus in the state from the initial props but when the prop is being updated the state is not being updated.

I suggest you avoid storing the focus in the local state and use the props directly.

key={ index }
icon={ alignment.icon }
label={ alignment.title }
data-subscript={ alignment.subscript }
Copy link
Member Author

Choose a reason for hiding this comment

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

This is not used.

}
const { latestPosts, focus } = this.state;

const alignments = [
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest we wait for #1019 to reuse the same toolbar here.

@@ -9,6 +9,9 @@ import { __ } from 'i18n';
*/
import { registerBlockType } from '../../api';
import { getLatestPosts } from './data.js';
import InspectorControls from '../../inspector-controls';
import IconButton from '../../../components/icon-button';
import classNames from 'classnames';
Copy link
Contributor

Choose a reason for hiding this comment

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

This is an external dependency, it should probably be moved to the top.

</div>
}
{ /* focus && */
<InspectorControls>
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really want to show up the alignments toolbar in the inspector? OR should it be it shown in the block controls? cc @jasmussen

Copy link
Member Author

Choose a reason for hiding this comment

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

This is to implement the designs in #1011 (comment)

) }
</ul>
<div>
{ 0 === latestPosts.length ?
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 rewrite this ternary like this?

{ condition
  ? (
    <Component />
  )
  : (
    <Component />
  )
}

Or maybe better

{ condition && (
    <Component />
) }
{ ! condition && (
    <Component />
) }

Or do you find it clearer as is?

Copy link
Member Author

Choose a reason for hiding this comment

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

The former is better than the latter I think, since it avoids duplicating the condition.

@lamosty
Copy link
Member

lamosty commented Jun 15, 2017

We are adding the "display post date" and "number of posts to show" in #1191. :D Glad that we didn't work on the same thing.

@westonruter
Copy link
Member Author

@lamosty but you're also adding alignment there too?

@lamosty
Copy link
Member

lamosty commented Jun 15, 2017

but you're also adding alignment there too?

Yeah but you did it better so I'm dropping alignment :)

@westonruter
Copy link
Member Author

Closing in favor of #1803.

@westonruter westonruter closed this Jul 8, 2017
@westonruter westonruter deleted the add/latest-posts-alignment branch July 8, 2017 07:22
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.

3 participants