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

Fix issues with new inserter #7074

Merged
merged 6 commits into from Jun 1, 2018

Conversation

@jasmussen
Contributor

jasmussen commented Jun 1, 2018

This PR fixes #7059.

It fixes an issue with IE11 where there was a horizontal scrollbar:

screen shot 2018-06-01 at 10 11 35

It changes and improves focus and hover styles to both look better overall, use mixins, and work in stacked contexts:

inserter

@jasmussen jasmussen self-assigned this Jun 1, 2018

@jasmussen jasmussen requested review from afercia, jorgefilipecosta and WordPress/gutenberg-core Jun 1, 2018

margin-right: 3px;
position: relative;
top: -2px;
left: -2px;
// Show a "stacked card" below an item that has children.

This comment has been minimized.

@jasmussen

jasmussen Jun 1, 2018

Contributor

This is why I CC'ed you for review, @jorgefilipecosta — I know you're working on a color thing in #7068, but in this PR I'm changing so that the colored car below the topmost is not a box-shadow, but a separate pseudo element. Can you work with this?

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Jun 1, 2018

Member

Thank for the ping @jasmussen. I did some research and unfortunately, we don't have a clean way to set styles for pseudo-elements like :after dynamically.
The options we have are creating a style element dynamically dangerouslySetInnerHTML and creating an empty element (that acts like the after ) where we apply the styles. I think this second option although not great is acceptable.

This comment has been minimized.

@jasmussen

jasmussen Jun 1, 2018

Contributor

Hmm. Like an extra empty dummy element in a block that has children? That could work.

But can you elaborate on why we can't set styles dynamically? Wouldn't you just output some CSS like this?

.woo.editor-inserter__item-icon:after {
	background: purple;
}

Where ".woo" would be the classname of whatever branded block is supplying the colors.

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Jun 1, 2018

Member

We would need to output the styles itself dynamically, not just pass a class something like:

.editor-inserter__item-icon:after {
	background: ${ color-set-by-the-block };
}

The way to output the styles dynamically with React would be using <style dangerouslySetInnerHTML={{..., and using dangerouslySetInnerHTML in React should be avoided if possible.

This comment has been minimized.

@mtias

mtias Jun 1, 2018

Contributor

If it's a background of a new element we can pass it as an inline style, no?

This comment has been minimized.

@mtias

mtias Jun 1, 2018

Contributor

Creating an empty element if the block has children seems the way to go.

This comment has been minimized.

@jasmussen

jasmussen Jun 1, 2018

Contributor

@jorgefilipecosta I pushed a change in 03894bd to use a separate element instead. Did I do it right, and can you work with that? Thanks.

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Jun 1, 2018

Member

Yes, we can use this version right now as it allows us to use the inline style.

@@ -94,6 +94,7 @@ class ItemList extends Component {
>
<span className="editor-inserter__item-icon">
<BlockIcon icon={ item.icon } />
{ item.hasChildBlocks ? <span className="editor-inserter__item-icon-stack"></span> : '' }

This comment has been minimized.

@mtias

mtias Jun 1, 2018

Contributor

Could be simplified:

item.hasChildBlocks && <span className="editor-inserter__item-icon-stack" />

This comment has been minimized.

@jasmussen

jasmussen Jun 1, 2018

Contributor

Done!

This comment has been minimized.

@aduth

aduth Jun 1, 2018

Member

Side-note: If you want to output nothing, prefer null instead of an empty string, since the latter actually creates a Node (Node.TEXT_NODE) in the DOM.

Demo: https://codepen.io/aduth/pen/mKeyzQ

@jorgefilipecosta

This seems to be working nicely (in11 and chrome) and the changes look good to me.

@jasmussen jasmussen added this to the 3.0 milestone Jun 1, 2018

@jasmussen jasmussen merged commit 0b32451 into master Jun 1, 2018

2 checks passed

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

@jasmussen jasmussen deleted the fix/inserter-issues branch Jun 1, 2018

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