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

Clean up empty links placeholders #1446

Closed
wants to merge 1 commit into from
Closed

Clean up empty links placeholders #1446

wants to merge 1 commit into from

Conversation

ellatrix
Copy link
Member

@ellatrix ellatrix commented Jun 26, 2017

  • After dismissing the link dialog and not entering a link, clean up the placeholder.
  • Fix errors on link input: Errors on link input #680. Fixed in 00c9bbe.
  • Avoid passing link node through formats.

@ellatrix ellatrix changed the title Clean up empty links + bug fix Clean up empty links placeholders Jun 26, 2017
@youknowriad
Copy link
Contributor

I'm seeing these two bugs, can you reproduce:

  • Adding a link, adding a second link, when validating the second link URL, the first one is updated instead and the second link is dropped
  • The auto-focus link input is not working

@ellatrix
Copy link
Member Author

ellatrix commented Jun 26, 2017

@youknowriad I can't reproduce the first issue. What URLs are you entering?

@youknowriad
Copy link
Contributor

@iseulde can't reproduce anymore :(. I should have recorded.

@ellatrix
Copy link
Member Author

Unsure why autofocus is not working. document.activeElement is still the input after render.

@youknowriad
Copy link
Contributor

@iseulde I think it's probably the removed setTimeout

@ellatrix
Copy link
Member Author

I checked, but doesn't look like it.

@ellatrix
Copy link
Member Author

@youknowriad This is super strange... Added the following:

componentDidUpdate() {
	console.log( this.state.linkValue, document.activeElement )
}

And it looks all good:

screenshot 2017-06-26 16 46 28

But the input field seems to focus super quick, then loses focus. Even after some time, document.activeElement is still the input though.

@youknowriad
Copy link
Contributor

youknowriad commented Jun 26, 2017

Maybe it's focusing another similar input, remaining somewhere hidden in the DOM? Really weird, I can't think of an explanation.

@ellatrix
Copy link
Member Author

No, when you hover over the element in the console, it highlights the right one.

@ellatrix
Copy link
Member Author

Having a hard time debugging this... Not sure at all what's going wrong.

@youknowriad
Copy link
Contributor

I'll plan to take a look later

@@ -141,12 +145,14 @@ class FormatToolbar extends Component {
} );
}

const isEditingLink = this.state.isEditingLink || this.state.linkValue === '#';
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the line causing the focus error. I can't understand why but if you drop the || this.state.linkValue === '#' it will work

Copy link
Contributor

Choose a reason for hiding this comment

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

What's the reason behind adding this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, this like is not needed anymore, but it doesn't fix the issue for me. The input field still won't autofocus.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, it's this line combined with the setTimeout

@gziolo
Copy link
Member

gziolo commented Sep 22, 2017

@youknowriad, still planning to look at this one? @iseulde is it still relevant? I can try to review it next wee :) It needs rebase.

@gziolo gziolo self-requested a review September 22, 2017 14:37
@youknowriad
Copy link
Contributor

I think this has been superseded by #2302

@gziolo
Copy link
Member

gziolo commented Sep 22, 2017

In that case I'm closing this one. @iseulde feel free to reopen if #2302 doesn't address all issues. This one updates also blocks/editable/index.js.

@gziolo gziolo closed this Sep 22, 2017
@gziolo gziolo deleted the fix/link-bugs branch September 22, 2017 14:43
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.

3 participants