Skip to content

Conversation

@tg-ephox
Copy link
Contributor

@tg-ephox tg-ephox commented Oct 30, 2017

Description

fixes: #3211

Moving the FormatToolbar out of the block caused the LinkDialog's offsetParent to be incorrect.

How Has This Been Tested?

Manual browser testing

Screenshots (jpeg or gifs if applicable):

Types of changes

Bug fix

Checklist:

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

@codecov
Copy link

codecov bot commented Oct 30, 2017

Codecov Report

Merging #3224 into master will decrease coverage by 0.06%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3224      +/-   ##
==========================================
- Coverage   31.36%   31.29%   -0.07%     
==========================================
  Files         223      227       +4     
  Lines        6393     6496     +103     
  Branches     1136     1160      +24     
==========================================
+ Hits         2005     2033      +28     
- Misses       3685     3739      +54     
- Partials      703      724      +21
Impacted Files Coverage Δ
blocks/editable/format-toolbar/index.js 6.38% <ø> (ø) ⬆️
blocks/editable/index.js 11.02% <0%> (ø) ⬆️
editor/sidebar/post-pending-status/index.js 43.75% <0%> (-14.59%) ⬇️
editor/sidebar/post-author/index.js 73.91% <0%> (-11.09%) ⬇️
blocks/editable/aria.js 100% <0%> (ø) ⬆️
editor/modes/visual-editor/block-list.js 0% <0%> (ø) ⬆️
editor/writing-flow/index.js 0% <0%> (ø) ⬆️
editor/block-mover/index.js 0% <0%> (ø) ⬆️
editor/post-pending-status/index.js 0% <0%> (ø)
editor/post-author/index.js 83.33% <0%> (ø)
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c5ebeca...9c62023. Read the comment docs.

@tg-ephox
Copy link
Contributor Author

Not sure if this is the best approach... it might make more sense to move the positioning styles to the Slot (or wrapper).

An alternative would be to add the current node's (within Editable) position to redux global state, although this could be costly as arrowing through a block's text would cause a lot to re-render (I believe its only fired on word or format change though).

One thing still broken is links within a table block, the overflow-x: auto style would need to be removed from wp-block-table for the link dialog to appear.

@tg-ephox tg-ephox requested a review from youknowriad October 30, 2017 07:30
Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

Nice Fix, Left a minor comment but this work great

{ MultilineTag ? <MultilineTag>{ placeholder }</MultilineTag> : placeholder }
</Tagname>
}
{ focus && <Slot name="Formatting.LinkDialog" /> }
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we rename the slot. I think it's meaning should be something related to its position and not what's inside it. Maybe something like Editable or Editable.Siblings

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Link Dialog's position appears randomly generated

3 participants