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 Test: Change icons to components with children slot #16551

Closed

Conversation

jeryj
Copy link
Contributor

@jeryj jeryj commented Jul 22, 2020

Instead of manually copy/pasting the icons over to the paid-icons file, this approach would change the icons we need to add a premium indicator symbol to into a component like:

export default function OpenTableIcon( { children, className } ) {
	return (
		<SVG
			className={ className ? className : null }
			...
		>
			<Path
				fill="#555d66"
				d="..."
			/>
			{ children }
		</SVG>
	);
}

Then, when using the icon in paid-icons.js:

import PaidSymbol from './paid-symbol';
import OpenTableIcon from '../../blocks/opentable/icon';

const openTablePaidIcon = (
	<OpenTableIcon className={ paidBlockClassName }>
		<PaidSymbol cx="24" cy="0" />
	</OpenTableIcon>
);

This removes the duplication of copy/pasted icon definitions when possible, but still allows for copy/pasting when we can't modify the icon. It does mean touching more files in more places, so could be harder to "undo" in the end if we did not need the { children } anymore.

I don't know if there are some contexts in which it would break the icon usage. Open Table uses the icon in the placeholder, and fortunately, this still works in that instance without any further changes:

const blockPlaceholder = (
		<Placeholder
			...
			icon={ <BlockIcon icon={ icon } /> }
			...
		>
			...
		</Placeholder>
	);

Open Table Icon rendered correctly in the block placeholder when the root icon is changed into the new format

Screen Shot 2020-07-22 at 9 45 21 AM

@stale
Copy link

stale bot commented Dec 25, 2020

This PR has been marked as stale. This happened because:

  • It has been inactive in the past 3 months.
  • It hasn’t been labeled `[Pri] Blocker`, `[Pri] High`.

No further action is needed. But it's worth checking if this PR has clear testing instructions, is it up to date with master, and it is still valid. Feel free to close this issue if you think it's not valid anymore — if you do, please add a brief explanation.

@stale stale bot added the [Status] Stale label Dec 25, 2020
@jeherve jeherve added [Block] OpenTable [Extension] Paid Blocks Functionality used to handle notices for paid blocks labels Jan 4, 2021
@stale stale bot removed the [Status] Stale label Jan 4, 2021
@jeherve
Copy link
Member

jeherve commented Jan 4, 2021

Closing this now that the base branch is not worked on anymore.

@jeherve jeherve closed this Jan 4, 2021
@kraftbj kraftbj deleted the update/add-paid-icon-symbol-to-root branch January 4, 2021 22:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] OpenTable [Extension] Paid Blocks Functionality used to handle notices for paid blocks Touches WP.com Files
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants