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

Sibling inserter: Fix the inserter position for floated blocks #4146

Merged
merged 1 commit into from
Jan 10, 2018

Conversation

youknowriad
Copy link
Contributor

closes #4142

This PR fixes the sibling inserter for floated blocks by moving the sibling inserter inside the BlockEdit component, that way we can position it relative to its container.

Testing instructions

  • Add a block button
  • Alight it right
  • Check that the inserter is showing up when you hover the border bottom of the block.

@youknowriad youknowriad added the [Feature] Inserter The main way to insert blocks using the + button in the editing interface label Dec 22, 2017
@youknowriad youknowriad self-assigned this Dec 22, 2017
@jasmussen
Copy link
Contributor

Impressive! This is working well for me. Can't speak for the code but experience wise this seems to fix it.

Does it also fix some of the float issues @gziolo reported? (Not that it has to) — #3870 #3869 #3868

@youknowriad
Copy link
Contributor Author

Unfortunately, it doesn't solve these issues. These will be much harder to solve since it's related to how "floating" works. We may have to think about "workarounds"

@jasmussen
Copy link
Contributor

Unfortunately, it doesn't solve these issues. These will be much harder to solve since it's related to how "floating" works. We may have to think about "workarounds"

Yep, they're still on my todo list, and someting I'd like to think about over the holidays. Perhaps we need to go back to a fixed-width main column, and use JavaScript to make full-wide images :(

@youknowriad
Copy link
Contributor Author

Perhaps we need to go back to a fixed-width main column, and use JavaScript to make full-wide images :(

I agree with this, we need a fixed-width main column.

@@ -439,6 +440,7 @@ export class BlockListBlock extends Component {
{ showUI && <BlockMobileToolbar uid={ block.uid } /> }
</div>
{ !! error && <BlockCrashWarning /> }
<BlockListSiblingInserter uid={ block.uid } />
Copy link
Member

Choose a reason for hiding this comment

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

I am not certain moving sibling inserter inside block-list will not have the undesirable side effect of making sibling inserter unusable for locked blocks cc: @gziolo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you mean by "locked blocks"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess you mean "template locking". In which case, this change doesn't change the current behavior which is: The Inserter component does not show anything if the template is locked. This component is included in the BlockListSiblingInserter component.

Copy link
Member

@jorgefilipecosta jorgefilipecosta Dec 22, 2017

Choose a reason for hiding this comment

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

In PR #3493 BlockListBlock was made extensible to allow the creation of Frozen/locked blocks. Blocks that are temporarily not editable because of co-editing for example. It looks like the hook allows us to "change" BlockListBlock, so in places that use/or we have plans to use this hook, we may be disabling sibling inserter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point! If that means the hook should be "smarter" or the hook should target BlockEdit instead?

Copy link
Member

Choose a reason for hiding this comment

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

In accordance with the conversation on PR #3493 adding the hook to BlockEdit does not fit the needs, because it would not be possible to lock the toolbar or the block transform mechanism. So we can make the hook smarter or we can try to add a component that wraps BlockListBlock and the sibling inserter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gziolo any thoughts on this?

Copy link
Member

Choose a reason for hiding this comment

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

@jorgefilipecosta thanks for raising this issue. Yes, it might be a bit inconvenient, but I'm sure we can find workarounds :) Don't treat it as a blocker as we can figure out solutions later.

So we can make the hook smarter or we can try to add a component that wraps BlockListBlock and the sibling inserter.

It seems like a wrapping div would work perfectly fine. I like the idea. I didn't think about it too much though :)

Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

Choose a reason for hiding this comment

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

I left a concern related to locked blocks. But other than that everything works as expected in my tests and I did not find any problem. Nice work 👍

@@ -211,20 +211,16 @@ class BlockList extends Component {
return (
<div>
{ !! blocks.length && <BlockListSiblingInserter /> }
{ flatMap( blocks, ( uid ) => [
{ flatMap( blocks, ( uid ) => (
Copy link
Member

Choose a reason for hiding this comment

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

This can be map now instead of flatMap if we're just returning a single value from the mapper.

@youknowriad youknowriad force-pushed the fix/floated-block-inserter branch from 51db93d to 4bf1e59 Compare January 5, 2018 15:05
@youknowriad
Copy link
Contributor Author

Ok this should be ready to go, merging

@youknowriad youknowriad merged commit 6005b45 into master Jan 10, 2018
@youknowriad youknowriad deleted the fix/floated-block-inserter branch January 10, 2018 14:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Inserter The main way to insert blocks using the + button in the editing interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Left & Right aligned Button Blocks don't show Insert Block icon
5 participants