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

Editor: Extract LinkContainer from FormatToolbar #9192

Merged
merged 3 commits into from Aug 22, 2018

Conversation

@gziolo
Member

gziolo commented Aug 21, 2018

Description

Extracted out of #9012.

This PR extracts LinkContainer to make it easier to provide React Native implementation for the parts that need to work differently on mobile devices.

How has this been tested?

npm test
npm run build

  1. Add paragraph block.
  2. Type some text.
  3. Select text and turn it into link.
  4. Type link url and save it.
  5. Edit just saved url and save it.
  6. Add more text and turn it into link.
  7. Go back to the previously added link and try to edit it.
  8. Remove added link.

All of those should work properly.

Types of changes

There should be no visual changes. It should be still possible to add/remove links from RichText powered fields like Paragraph block.

Checklist:

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

@gziolo gziolo added this to the 3.7 milestone Aug 21, 2018

@gziolo gziolo self-assigned this Aug 21, 2018

@gziolo gziolo requested review from SergioEstevao and iseulde Aug 21, 2018

@SergioEstevao

This is looking good and it will make it easier to port the toolbar to RN.

@gziolo

This comment has been minimized.

Show comment
Hide comment
@gziolo

gziolo Aug 21, 2018

Member

I added fix in e08af7b which resolves the issue with race condition between props update and internal state changes. To make it work properly the derived state always set linkValue based on formats prop and it changes explicitly only when the user types.

Member

gziolo commented Aug 21, 2018

I added fix in e08af7b which resolves the issue with race condition between props update and internal state changes. To make it work properly the derived state always set linkValue based on formats prop and it changes explicitly only when the user types.

@gziolo gziolo requested a review from WordPress/gutenberg-core Aug 21, 2018

@@ -1000,7 +999,7 @@ RichText.contextTypes = {
};
RichText.defaultProps = {
formattingControls: DEFAULT_FORMATS,
formattingControls: FORMATTING_CONTROLS.map( ( { format } ) => format ),

This comment has been minimized.

@gziolo

gziolo Aug 21, 2018

Member

@iseulde - as you pointed out, we don't need this constant as we can produce it from the default FORMATTING_CONTROLS.

@gziolo

gziolo Aug 21, 2018

Member

@iseulde - as you pointed out, we don't need this constant as we can produce it from the default FORMATTING_CONTROLS.

@noisysocks

Looks good! We're definitely overdue to break up this component 🙂

I'm wondering if we should move all of the code that knows about links out of FormatToolbar?

@@ -140,19 +107,16 @@ class FormatToolbar extends Component {
}
addLink() {

This comment has been minimized.

@noisysocks

noisysocks Aug 22, 2018

Member

Why not move these methods to LinkContainer?

@noisysocks

noisysocks Aug 22, 2018

Member

Why not move these methods to LinkContainer?

This comment has been minimized.

@gziolo

gziolo Aug 22, 2018

Member

It should be now possible as I removed the usage of setState in both methods shared with toolbarControls variable. I will double check if this is expected on mobile and open a follow-up PR 👍

@gziolo

gziolo Aug 22, 2018

Member

It should be now possible as I removed the usage of setState in both methods shared with toolbarControls variable. I will double check if this is expected on mobile and open a follow-up PR 👍

@@ -140,19 +107,16 @@ class FormatToolbar extends Component {
}
addLink() {

This comment has been minimized.

@gziolo

gziolo Aug 22, 2018

Member

It should be now possible as I removed the usage of setState in both methods shared with toolbarControls variable. I will double check if this is expected on mobile and open a follow-up PR 👍

@gziolo

gziolo Aug 22, 2018

Member

It should be now possible as I removed the usage of setState in both methods shared with toolbarControls variable. I will double check if this is expected on mobile and open a follow-up PR 👍

@@ -164,8 +129,6 @@ class FormatToolbar extends Component {
rel: this.state.opensInNewWindow ? 'noreferrer noopener' : null,
value,
} } );
this.setState( { linkValue: value, isEditingLink: false } );
if ( ! this.props.formats.link.value ) {

This comment has been minimized.

@gziolo

gziolo Aug 22, 2018

Member

We call this.props.onChange so we don't need to update state explicitly because getDerivedStateFromProps will handle it properly.

@gziolo

gziolo Aug 22, 2018

Member

We call this.props.onChange so we don't need to update state explicitly because getDerivedStateFromProps will handle it properly.

@gziolo

This comment has been minimized.

Show comment
Hide comment
@gziolo

gziolo Aug 22, 2018

Member

I'm wondering if we should move all of the code that knows about links out of FormatToolbar?

As commented already. It wasn't possible initially because both Toolbar and LinkContainer shared the same methods which depended on shared state. It's no longer an issue so we can take another step. I would prefer to do it in its own PR so we could unblock React Native implementation (#9012).

Member

gziolo commented Aug 22, 2018

I'm wondering if we should move all of the code that knows about links out of FormatToolbar?

As commented already. It wasn't possible initially because both Toolbar and LinkContainer shared the same methods which depended on shared state. It's no longer an issue so we can take another step. I would prefer to do it in its own PR so we could unblock React Native implementation (#9012).

@noisysocks

Looks good to me!

@gziolo gziolo merged commit 649320e into master Aug 22, 2018

2 checks passed

codecov/project 50.91% (+0.01%) compared to 0e8c5e6
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@gziolo gziolo deleted the update/split-formatting-toolbars branch Aug 22, 2018

@iseulde iseulde referenced this pull request Aug 22, 2018

Closed

Rich text formatting filter #6642

0 of 4 tasks complete

@gziolo gziolo referenced this pull request Aug 22, 2018

Merged

Implement toolbar on React Native #9012

0 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment