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 custom placeholder icon #5965

Merged
merged 1 commit into from
Apr 12, 2018

Conversation

ajitbohra
Copy link
Member

Description

Currently Placeholder component does not support custom icon like IconButton. Allowing custom icon will make the component more flexible.

How Has This Been Tested?

Manually

Screenshots (jpeg or gifs if applicable):

placeholder-icon

Types of changes

Nonbreaking component change

Checklist:

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

Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

Nice 👍

Evidenced by this bug, maybe we need future consolidation of this logic into an "Icon" component. Not necessary, but an idea.

@ajitbohra
Copy link
Member Author

@aduth same thought Icon Component to dry the icons implementation. Will also help in code consistency have identified few places which can be refactored. Planning to work on the PR for the same.

@aduth aduth added the [Feature] UI Components Impacts or related to the UI component system label Apr 12, 2018
@aduth aduth force-pushed the add/custom-placeholder-icon branch from 4faccc3 to 88252af Compare April 12, 2018 19:13
@aduth aduth merged commit 49d4d7f into WordPress:master Apr 12, 2018
@ajitbohra
Copy link
Member Author

@aduth

Was looking to drying up Icon:

Currently, we are allowing the custom icon for:

  • ButtonIcon
  • Placeholder
  • BlockIcon

Have noticed we already have a reusable component for the same BlockIcon either we can use this or rename it to Icon to be more generic and refactoring code referencing it. Also, we can move it to Components your thoughts.

@ajitbohra ajitbohra deleted the add/custom-placeholder-icon branch April 25, 2018 13:08
@aduth
Copy link
Member

aduth commented Apr 25, 2018

Yeah, good call, BlockIcon looks like it could easily be generalized as Icon. There's nothing block-specific about it. Moving to components sounds reasonable. Since it's exposed as part of the public interface, we could either keep it (as a simple pass-through to Icon) or deprecate it.

@ajitbohra
Copy link
Member Author

@aduth deprecating it sounds much better will work on the initial PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] UI Components Impacts or related to the UI component system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants