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 BlockIconWithColors component; Use the component in the inspector. #7263

Merged
merged 3 commits into from Jun 20, 2018

Conversation

Projects
None yet
5 participants
@jorgefilipecosta
Member

jorgefilipecosta commented Jun 11, 2018

Description

This PR implements a new component BlockIconWithColors that displays a block icon with foreground an background colors applied.
Right now the component is used when showing the parent on the inserter, and on the inspector.

How has this been tested?

Test blocks: https://gist.github.com/jorgefilipecosta/7fa80d677486bfae53d6b86977098464

I added the test blocks by pasting the gist on the console.

I added the product block, selected it and checked on the inserter that product block appears as before.

I verified on the inspector that product block now has the brand colors.

I verified that core blocks on the inspector appear with a small design change and everything works as expected.

Screenshots

screen shot 2018-06-11 at 15 04 21

screen shot 2018-06-11 at 15 04 10

screen shot 2018-06-11 at 15 01 52

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Jun 11, 2018

Very lovely! I pushed a tiny fix to make the card a teensy bit taller, allowing for a bit of padding above and below:

screen shot 2018-06-11 at 16 13 02

👍 👍 from a design pov!

Also CC: @karmatosed, we talked about this just now.

@jorgefilipecosta jorgefilipecosta requested a review from WordPress/gutenberg-core Jun 11, 2018

import './style.scss';
import BlockIcon from '../block-icon';
export default function BlockIconWithColors( { iconObject, className, ...props } ) {

This comment has been minimized.

@youknowriad

youknowriad Jun 11, 2018

Contributor

I wonder if this should be a separate component or just a prop in BlockIcon?

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Jun 11, 2018

Member

Block icon receives just the icon source, and it can already be complex (a function, a component, an element or string). I feel adding another type (an object with a specific structure) will make things even more complex.

This comment has been minimized.

@youknowriad

youknowriad Jun 11, 2018

Contributor

I guess what I don't like is that the BlockIcon component doesn't support the same things as the icon attribute of a block. From someone external to Gutenberg, you shouldn't have to know the shape of the icon you're using and which component supports it. You just want to show the block icon so you just feed it to the BlockIcon component.

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Jun 18, 2018

Member

Hi @youknowriad, thank you for the feedback. I updated the code and now we use only BlockIcon component, let me know if you prefer this version.

@youknowriad

LGTM 👍

@jorgefilipecosta jorgefilipecosta requested a review from karmatosed Jun 19, 2018

@jorgefilipecosta

This comment has been minimized.

Member

jorgefilipecosta commented Jun 19, 2018

@karmatosed, is it ok to merge this changes regarding the UX point of view?

@jorgefilipecosta jorgefilipecosta added this to the 3.1 milestone Jun 19, 2018

@karmatosed

Approving the changes, thanks everyone!

@jorgefilipecosta jorgefilipecosta merged commit 1770317 into master Jun 20, 2018

2 checks passed

codecov/project 46.87% (+<.01%) compared to 657c368
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jorgefilipecosta jorgefilipecosta deleted the add/BlockIconWithColors branch Jun 20, 2018

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