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

FIX: Link Dialog's position appears randomly generated #3224

Merged
merged 2 commits into from
Oct 31, 2017

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

@@ -709,6 +709,7 @@ export default class Editable extends Component {
{ 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
2 participants