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.

Sign up for GitHub

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

Jump to bottom

URL Input: Use Popover instead of custom positionning and custom modals #6911

Merged
merged 5 commits into from
May 24, 2018
Merged

URL Input: Use Popover instead of custom positionning and custom modals #6911

merged 5 commits into from
May 24, 2018

Conversation

youknowriad
Copy link
Contributor

@youknowriad youknowriad commented May 23, 2018 •
edited
Loading

This PR updates the URL Input and the format toolbar to use the Popover component to display the link modal. The Popover contain behavior to adapt the width and the position to the available space which will fix a lot of issues we have with link modals on mobile.

Testing instructions

  • Test the link inputs in the several places we use them (format toolbar, inline format toolbar, image block toolbar, button block...)

closes #3941 #4447 #6183 #6393 #6810

@youknowriad youknowriad self-assigned this May 23, 2018
@jasmussen
Copy link
Contributor

closes #3941 #4447 #6183 #6393 #6810

🤘

This is a big improvement.

I wish we could, like TinyMCE does, show a little shadow of what's selected in the text, even if your text focus is elsewhere. But that's for separate polish.

One thing: this doesn't seem to work when the toolbar is fixed to the top.

@youknowriad
Copy link
Contributor Author

One thing: this doesn't seem to work when the toolbar is fixed to the top.

Good catch, I'll take a look

@jasmussen
Copy link
Contributor

One thing: this doesn't seem to work when the toolbar is fixed to the top.

Actually, scratch that. I don't know what it was that I was seeing, but a hard reload made this work. Stellar work!

Can you move the popover upwards about 10px? that would make it feel more connected with the link.

@youknowriad youknowriad mentioned this pull request May 23, 2018
Merged
jasmussen
jasmussen approved these changes May 23, 2018
View reviewed changes
Copy link
Contributor

@jasmussen jasmussen left a comment

Choose a reason for hiding this comment

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

After the latest fix, this is cool from a design perspective.

ellatrix
ellatrix reviewed May 23, 2018
View reviewed changes
editor/components/rich-text/index.js Outdated

return {
top: position.top - containerPosition.top + ( position.height ) + toolbarOffset.top,
left: position.left - containerPosition.left + ( position.width / 2 ) + toolbarOffset.left,
top: position.top - containerPosition.top + ( position.height ),
Copy link
Member

Choose a reason for hiding this comment

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

Tiny thing: unsure why there are brackets here, but they were here before.

@ellatrix
Copy link
Member

Seeing the following error when adding the target attribute, and I don't see this error in master?

index.js:292 Uncaught Error: Invalid assumption: expected selection to be within the autocomplete container
    at Autocomplete.getCursor (index.js:292)
    at Autocomplete.search (index.js:443)
    at HTMLUnknownElement.callCallback (react-dom.cca5b42c.js:140)
    at Object.invokeGuardedCallbackDev (react-dom.cca5b42c.js:178)
    at Object.invokeGuardedCallback (react-dom.cca5b42c.js:227)
    at Object.invokeGuardedCallbackAndCatchFirstError (react-dom.cca5b42c.js:241)
    at executeDispatch (react-dom.cca5b42c.js:604)
    at executeDispatchesInOrder (react-dom.cca5b42c.js:626)
    at executeDispatchesAndRelease (react-dom.cca5b42c.js:724)
    at executeDispatchesAndReleaseTopLevel (react-dom.cca5b42c.js:735)

@youknowriad
Copy link
Contributor Author

@iseulde I'm seeing the same issue on master

@noisysocks
Copy link
Member

I wish we could, like TinyMCE does, show a little shadow of what's selected in the text, even if your text focus is elsewhere. But that's for separate polish.

There's an issue for that: #6156

noisysocks
noisysocks previously requested changes May 24, 2018
View reviewed changes
Copy link
Member

@noisysocks noisysocks left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I love this ❤️

I see just one visual regression. Before there was a nice focus outline on the inline link input:

screen shot 2018-05-24 at 09 59 22

But now that's gone:

screen shot 2018-05-24 at 10 01 23

editor/components/rich-text/format-toolbar/index.js Outdated
@@ -98,6 +100,7 @@ class FormatToolbar extends Component {
settingsVisible: false,
opensInNewWindow: !! nextProps.formats.link && !! nextProps.formats.link.target,
linkValue: '',
popoverId: this.state.popoverId + 1,
Copy link
Member

Choose a reason for hiding this comment

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

Could we re-use this.props.selectedNodeId instead of introducing more state? Note that RichText increments this in much the same way.

https://github.com/WordPress/gutenberg/blob/master/editor/components/rich-text/index.js#L708

@youknowriad youknowriad dismissed noisysocks’s stale review May 24, 2018 09:53

Thanks for the review, I didn't notice the selectedNodeId. Good advice 👍👍

@youknowriad youknowriad merged commit 5ebefc3 into master May 24, 2018
@youknowriad youknowriad deleted the update/link-modal branch May 24, 2018 12:25
@youknowriad youknowriad added this to the 3.0 milestone May 30, 2018
@afercia afercia mentioned this pull request Jul 28, 2018
Closed
aduth
aduth reviewed Jul 30, 2018
View reviewed changes
components/popover/index.js
@@ -219,6 +220,7 @@ class Popover extends Component {
'is-' + xAxis,
{
'is-mobile': isMobile,
'no-arrow': noArrow,
Copy link
Member

@aduth aduth Jul 30, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

This is not very consistent with modifier class naming which are otherwise boolean-ish. I might have preferred something like has-no-arrow or is-without-arrow or is-no-arrow.

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 like how you see a comment from Andrew from time to time on old PRs :P

Anyway, I just pushed a fix to master (accidentally, I thought I was on a branch 😬 )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

3ce3fee

Copy link
Member

Choose a reason for hiding this comment

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

I enjoy to surf the git blame 😄

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

@ellatrix ellatrix ellatrix left review comments

@aduth aduth aduth left review comments

@jasmussen jasmussen jasmussen approved these changes

@noisysocks noisysocks noisysocks left review comments

Assignees

@youknowriad youknowriad

Labels
None yet
Projects
None yet
Milestone
3.0
Development

Successfully merging this pull request may close these issues.

Editor: Anchor's popover displays partially off the screen
5 participants
@youknowriad @jasmussen @ellatrix @noisysocks @aduth