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: Empty Links shows on frontend #2302

Merged
merged 9 commits into from Aug 15, 2017

Conversation

3 participants
@tg-ephox
Contributor

tg-ephox commented Aug 9, 2017

Fixes #2080

Changed the Toolbar to only create a link when the URL has been entered, instead of creating the link up front and then setting the URL.

I needed to lift the state for the link functionality from FormatToolbar to Editable as this needs to be reset from onNodeChange. This allowed FormatToolbar to be simplified a lot as we don't have to guess what has happened in componentWillReceiveProps.

A remaining issue is that when typing into UrlInput, the cursor position is reset to the end if typing in the middle. This is caused by the updateSuggestions function which does an api call and sets the state causing a re-render. I believe that moving this to a separate component will fix the issue. (Using the separate component's render cycle instead of re-rendering the input which is confusing react as to where the cursor should be)

Fixes #1069 (I put this in the same PR as its right in the same area)

Added CSS class and custom selector to TinyMCE so when the Toolbar is used, it doesn't loose focus. Also before applying formatting, focus the TinyMCE instance. This meant that the bookmark was no longer needed in state.

@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Aug 9, 2017

Codecov Report

Merging #2302 into master will increase coverage by <.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #2302      +/-   ##
=========================================
+ Coverage    25.9%   25.9%   +<.01%     
=========================================
  Files         157     157              
  Lines        4849    4848       -1     
  Branches      820     817       -3     
=========================================
  Hits         1256    1256              
- Misses       3033    3035       +2     
+ Partials      560     557       -3
Impacted Files Coverage Δ
blocks/editable/format-toolbar/index.js 7.5% <0%> (+0.52%) ⬆️
blocks/editable/index.js 10.87% <0%> (-0.1%) ⬇️

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 961e987...620892a. Read the comment docs.

codecov bot commented Aug 9, 2017

Codecov Report

Merging #2302 into master will increase coverage by <.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #2302      +/-   ##
=========================================
+ Coverage    25.9%   25.9%   +<.01%     
=========================================
  Files         157     157              
  Lines        4849    4848       -1     
  Branches      820     817       -3     
=========================================
  Hits         1256    1256              
- Misses       3033    3035       +2     
+ Partials      560     557       -3
Impacted Files Coverage Δ
blocks/editable/format-toolbar/index.js 7.5% <0%> (+0.52%) ⬆️
blocks/editable/index.js 10.87% <0%> (-0.1%) ⬇️

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 961e987...620892a. Read the comment docs.

tg-ephox added some commits Aug 9, 2017

@tg-ephox tg-ephox requested a review from aduth Aug 9, 2017

@youknowriad

This works great, but I think we should still keep the local state in the FormatToolbar. A state that is not used in a component but just passed as a prop to the child component should leave in the child component, don't you think?

Show outdated Hide outdated blocks/editable/format-toolbar/index.js
Show outdated Hide outdated blocks/editable/index.js
@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Aug 11, 2017

Member

Added CSS class and custom selector to TinyMCE so when the Toolbar is used, it doesn't loose focus.

Can you explain why this is necessary? I'm a bit reluctant about this change for a few reasons:

  • The Editable becomes aware of and dependent upon the existence of the Editor wrapper assigning this class
  • So far as distinct Editables, and blocks not leveraging Editable, it would seem to me that we would want blur to occur when leaving any individual Editable
Member

aduth commented Aug 11, 2017

Added CSS class and custom selector to TinyMCE so when the Toolbar is used, it doesn't loose focus.

Can you explain why this is necessary? I'm a bit reluctant about this change for a few reasons:

  • The Editable becomes aware of and dependent upon the existence of the Editor wrapper assigning this class
  • So far as distinct Editables, and blocks not leveraging Editable, it would seem to me that we would want blur to occur when leaving any individual Editable

@anna-harrison anna-harrison added this to To Do in Ephox Team Aug 14, 2017

@anna-harrison anna-harrison moved this from To Do to In Progress in Ephox Team Aug 14, 2017

@tg-ephox

This comment has been minimized.

Show comment
Hide comment
@tg-ephox

tg-ephox Aug 14, 2017

Contributor

@aduth @youknowriad I removed the CSS class and selector, I was recommended to try this in order that the editor keeps its selection but it seems that focussing the editor before applying/removing formatting fixes this anyway. Have also moved the FormatToolbar state back and added a selectedNodeId so that FormatToolbar can track node changes.

Contributor

tg-ephox commented Aug 14, 2017

@aduth @youknowriad I removed the CSS class and selector, I was recommended to try this in order that the editor keeps its selection but it seems that focussing the editor before applying/removing formatting fixes this anyway. Have also moved the FormatToolbar state back and added a selectedNodeId so that FormatToolbar can track node changes.

tg-ephox added some commits Aug 14, 2017

@youknowriad

It looks like this is working great. 👍

When I worked on the UrlInput the first time, I had to do add this convert_urls: false, to the TinyMCE settings to avoid JS errors when using empty URLs. Maybe we can remove this option now?

@tg-ephox

This comment has been minimized.

Show comment
Hide comment
@tg-ephox

tg-ephox Aug 15, 2017

Contributor

@youknowriad I tried removing the convert_urls setting from TinyMCE and didn't see any difference... Although UrlInput forces a complete URL to be entered anyway so this option might not be relevant, but I have left it there for now.

Contributor

tg-ephox commented Aug 15, 2017

@youknowriad I tried removing the convert_urls setting from TinyMCE and didn't see any difference... Although UrlInput forces a complete URL to be entered anyway so this option might not be relevant, but I have left it there for now.

@tg-ephox tg-ephox merged commit c29caae into master Aug 15, 2017

3 checks passed

codecov/project 25.9% (+<.01%) compared to 961e987
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@tg-ephox tg-ephox deleted the fix/2080-empty-link-shows branch Aug 15, 2017

@tg-ephox tg-ephox moved this from In Progress to Done in Ephox Team Aug 15, 2017

@anna-harrison anna-harrison moved this from Done to Logged in JIRA in Ephox Team Aug 28, 2017

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