[RNMobile] Set and reference heading anchors #27935

dcalhoun · 2020-12-30T21:01:55Z

Description

Address wordpress-mobile/gutenberg-mobile#1816 by adding the ability to set and reference Heading block anchors

Guntenberg Mobile: Set and reference heading anchors wordpress-mobile/gutenberg-mobile#2947
WordPress iOS: [Gutenberg] Set and reference heading anchors wordpress-mobile/WordPress-iOS#15567
WordPress Android: [Gutenberg] Set and reference heading anchors wordpress-mobile/WordPress-Android#13705

How has this been tested?

Edit a post and add Heading block.
Tap block settings button (i.e. settings cog icon).
Set value for "HTML Anchor" control.
Dismiss block settings.
Add Paragraph block with text including a link to the HTML Anchor from
earlier step.
Publish post.
Visit published post in a web browser.
Tap link.

Outcome: Browser should scroll to Heading anchor.

Screenshots

UI Examples

iOS	Android

Types of changes

New feature (non-breaking change which adds functionality)

Checklist

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
My code has proper inline documentation.
I've included developer documentation if appropriate.
I've updated all React Native files affected by any refactorings/renamings in this PR.

hypest · 2021-01-08T15:22:37Z

👋 David! May I suggest changing the "prefix" in the PR title to [RNMobile] instead of just [Mobile]? "RNMobile" is not a hugely better prefix but, it's rather unique at that and disambiguating between mobile-web and native-mobile. Thanks!

dcalhoun · 2021-01-13T14:56:06Z

@hypest I do not have permissions to manage the reviewers list. After consideration, including four members is likely unnecessary. Would you please update the reviewers to list to be @ceyhun and @talldan please?

@ceyhun would you please review that my implementation works as expected and is a sound approach?

@talldan would you be willing to review my work to ensure my implementation does not disrupt any web experiences or violate any general project coding practices?

talldan · 2021-01-14T02:31:35Z

@dcalhoun Thanks for the ping.

I tested this and on the web I'm seeing two way to set an anchor on the heading block now:

The UI for the anchor in the advanced section is added to some blocks by the supports setting:

gutenberg/packages/block-library/src/heading/block.json

Line 25 in e1c3dcf

"anchor": true,

And the code that picks up that setting and adds the UI is here (added using filters):

gutenberg/packages/block-editor/src/hooks/anchor.js

Lines 64 to 113 in e1c3dcf

    
           export const withInspectorControl = createHigherOrderComponent( 
        
           	( BlockEdit ) => { 
        
           		return ( props ) => { 
        
           			const hasAnchor = hasBlockSupport( props.name, 'anchor' ); 
        
           			if ( hasAnchor && props.isSelected ) { 
        
           				return ( 
        
           					<> 
        
           						<BlockEdit { ...props } /> 
        
           						<InspectorAdvancedControls> 
        
           							<TextControl 
        
           								className="html-anchor-control" 
        
           								label={ __( 'HTML anchor' ) } 
        
           								help={ 
        
           									<> 
        
           										{ __( 
        
           											'Enter a word or two — without spaces — to make a unique web address just for this heading, called an “anchor.” Then, you’ll be able to link directly to this section of your page.' 
        
           										) } 
        
           										<ExternalLink 
        
           											href={ 
        
           												'https://wordpress.org/support/article/page-jumps/' 
        
           											} 
        
           										> 
        
           											{ __( 'Learn more about anchors' ) } 
        
           										</ExternalLink> 
        
           									</> 
        
           								} 
        
           								value={ props.attributes.anchor || '' } 
        
           								onChange={ ( nextValue ) => { 
        
           									nextValue = nextValue.replace( 
        
           										ANCHOR_REGEX, 
        
           										'-' 
        
           									); 
        
           									props.setAttributes( { 
        
           										anchor: nextValue, 
        
           									} ); 
        
           								} } 
        
           								autoComplete="off" 
        
           							/> 
        
           						</InspectorAdvancedControls> 
        
           					</> 
        
           				); 
        
           			} 
        
           			return <BlockEdit { ...props } />; 
        
           		}; 
        
           	}, 
        
           	'withInspectorControl' 
        
           );

I think if you're looking to add anchor support on mobile, you'd probably want to use the same system to ensure it automatically works across all blocks that support the anchor setting.

ceyhun

Just saw the comment from Daniel. I guess the mobile implementation will be different from the web one, so I think it makes sense to review and test after that change. Feel free to ping me again 🙇

hypest · 2021-01-15T11:45:19Z

I tested this and on the web I'm seeing two way to set an anchor on the heading block now:

Aha, nice find there Daniel!

I think if you're looking to add anchor support on mobile, you'd probably want to use the same system to ensure it automatically works across all blocks that support the anchor setting.

👋 @dcalhoun , just wanted to add that in terms of scope, it'd be cool if we can limit to the heading block for now, but still have an implementation that lends itself to expanding to other blocks. That said, I'm not sure if that'd be more practical or quicker than just go for all blocks as Daniel mentions, but if we can break up the scope in such a way that we land things in separate PRs it can help with moving forward in a nice pace. Thanks!

dcalhoun · 2021-01-15T13:33:40Z

I tested this and on the web I'm seeing two way to set an anchor on the heading block now:

Aha, nice find there Daniel!

Indeed. Thank you for reviewing, Daniel. You have opened my eyes to a much wider view of the code base and it's incredibly helpful.

it'd be cool if we can limit to the heading block for now, but still have an implementation that lends itself to expanding to other blocks. That said, I'm not sure if that'd be more practical or quicker than just go for all blocks as Daniel mentions, but if we can break up the scope in such a way that we land things in separate PRs it can help with moving forward in a nice pace.

@hypest whether it is more or less work to scope enabling anchors for headings will likely depend upon the response to my questions for Thomas. If we is OK with using a generic heading for the bottom sheet, then enabling anchor hook for all blocks will likely "just work out of the box."

That said, I can go ahead and execute enabling anchors for Heading blocks only to sustain project momentum.

dcalhoun · 2021-01-15T17:20:25Z

@talldan @ceyhun I have addressed the initial feedback. I believe this PR is now ready for review again. Thank you!

talldan

Things look good from a web point of view. Happy for someone more familiar with mobile to do a test there and give the final thumbs up 👍.

talldan · 2021-01-20T06:59:34Z

packages/block-editor/src/hooks/anchor.js

+							</InspectorAdvancedControls>
+						) }
+						{ ! isWeb && props.name === 'core/heading' && (
+							<InspectorControls>


At some point it would be cool to look at supporting InspectorAdvancedControls on mobile (guess it's not rendered anywhere at the moment though).

There are a few other things that tend to be rendered in this panel/slot on the web, the main other item is the Custom Class Name field (https://github.com/WordPress/gutenberg/blob/master/packages/block-editor/src/hooks/custom-class-name.js):

Those things could always be prevented from rendering on mobile in the same way you have here if you want a more gradual roll-out.

I agree. Ideally we reuse InspectorAdvancedControls for mobile once we have a design for displaying its entirety. Thomas and I had a discussion about the "Advanced" section, but opted to target only the anchor input for the initial implementation.

talldan · 2021-01-20T07:01:45Z

packages/block-editor/src/hooks/anchor.js

+						) }
+						{ ! isWeb && props.name === 'core/heading' && (
+							<InspectorControls>
+								<PanelBody title={ __( 'Heading settings' ) }>


'Heading settings' might not be very scalable for the future, as other features that get added to the block inspector for Heading can't be put inside this panel. There's no way to access it from the Block's edit function.

Agreed, we'll need a more scalable heading in the future. Thomas requested that we use the "Heading settings" title for the time being.

Thomas requested

My comment below is actually covering the process here too, thanks!

talldan · 2021-01-20T07:08:03Z

packages/block-editor/src/hooks/anchor.js

+								{ textControl }
+							</InspectorAdvancedControls>
+						) }
+						{ ! isWeb && props.name === 'core/heading' && (


Is there a plan to remove the restriction to just the one block?

I'd hope that as long as TextControl works well on mobile, it should be fairly straightforward to support this on all blocks that declare support for an HTML Anchor.

Yes, I believe the plan is to implement other blocks in the future. For the time being, Thomas and Stefanos requested that we scope the work to Heading blocks.

From my testing, you are correct that wider anchor support is straightforward.

requested

Just a quick note on the process, as "requested" doesn't represent it very well: The way we work, at least on the native mobile apps, we make sure people collaborate by sharing information and context to design/implement solutions, including carving out the plan to get there.

We try to keep PR scope to the minimum so we can move and land land them in small but predictable steps (even if, say, landing means merging to a feature branch). We re-evaluate our plans as we go, when new info arises, like for example when we understand scope is actually too big for a single PR.

In our case here with the anchor feature, limiting to just the "Heading" block helps us pin down the implementation on mobile, add value by shipping the feature to the users, all while we making plans for the next steps.

Hope this helps, thanks!

@hypest thank you for the feedback and context.

Maybe we can add a comment here linking the issue where we plan to implement InspectorAdvancedControls and/or enable anchors for all the other blocks?

talldan · 2021-01-20T07:12:16Z

packages/block-editor/README.md

@@ -394,7 +394,7 @@ Undocumented declaration.

 _Related_

-   <https://github.com/WordPress/gutenberg/blob/HEAD/packages/block-editor/src/components/inspector-controls-advanced/README.md>
+-   <https://github.com/WordPress/gutenberg/blob/HEAD/packages/block-editor/src/components/inspector-advanced-controls/README.md>


good catch!

dcalhoun · 2021-01-20T19:39:57Z

@talldan thank you again for the solid review. You shared great thoughts and questions. I have created #28363 to allow us to continue a discussion on when/how we might add wider support anchors on mobile.

ceyhun

Sorry for the late review @dcalhoun, it LGTM 👍 Tested on iOS and Android and everything is working as expected. Thanks!

ceyhun · 2021-01-26T14:43:12Z

packages/block-editor/src/hooks/anchor.js

+								{ textControl }
+							</InspectorAdvancedControls>
+						) }
+						{ ! isWeb && props.name === 'core/heading' && (


Maybe we can add a comment here linking the issue where we plan to implement InspectorAdvancedControls and/or enable anchors for all the other blocks?

packages/react-native-editor/CHANGELOG.md

Address design feedback.

Previous approach ignored web implementation and resulted in double anchor controls for web editor. This leverages the existing anchor hook to fix the issue and share more code.

Address code review feedback.

dcalhoun · 2021-01-26T17:19:24Z

@ceyhun thank you for the review. I pushed a new commit to address your feedback. I believe this PR is ready to merge. I do not have the privileges required to rerun the failing CI tests or merge the PR. Would you be willing to help me do that please?

ceyhun · 2021-01-26T17:39:15Z

I restarted the End-to-End Tests / Admin jobs, but they are already failing on master, so I think it'll be OK to merge this PR even if they don't pass.

github-actions bot added the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label Dec 30, 2020

dcalhoun force-pushed the rnmobile/set-and-reference-heading-anchors branch from d1418d4 to d79c73a Compare January 6, 2021 13:39

dcalhoun mentioned this pull request Jan 7, 2021

Add support for setting anchors and referring to them from links wordpress-mobile/gutenberg-mobile#1816

Closed

dcalhoun marked this pull request as ready for review January 8, 2021 02:18

dcalhoun requested review from ajitbohra and talldan as code owners January 8, 2021 02:18

guarani requested review from ceyhun and jd-alexander January 8, 2021 13:42

dcalhoun changed the title ~~[Mobile] Set and reference heading anchors~~ [RNMobile] Set and reference heading anchors Jan 8, 2021

hypest removed request for ajitbohra and jd-alexander January 13, 2021 16:48

dcalhoun force-pushed the rnmobile/set-and-reference-heading-anchors branch from 455a9ca to 2d4c648 Compare January 13, 2021 22:47

ceyhun reviewed Jan 15, 2021

View reviewed changes

dcalhoun requested a review from ellatrix as a code owner January 15, 2021 15:44

dcalhoun requested a review from ceyhun January 15, 2021 17:21

dcalhoun force-pushed the rnmobile/set-and-reference-heading-anchors branch from f653167 to 61a2168 Compare January 19, 2021 21:45

talldan reviewed Jan 20, 2021

View reviewed changes

talldan added [Block] Heading Affects the Headings Block [Type] Enhancement A suggestion for improvement. Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) labels Jan 20, 2021

dcalhoun mentioned this pull request Jan 20, 2021

[RNMobile] Support setting anchors for all eligible blocks #28363

Closed

ceyhun approved these changes Jan 26, 2021

View reviewed changes

dcalhoun added 8 commits January 26, 2021 10:11

Allow setting heading anchor

08286ad

Update change log

1e7ab71

Disable auto capitalization for anchor text input

0cbeba4

Update title and placeholder text

2ed4169

Address design feedback.

Fix broken documentation link

e65d643

Refactor heading anchors for mobile

b3aaa45

Previous approach ignored web implementation and resulted in double anchor controls for web editor. This leverages the existing anchor hook to fix the issue and share more code.

Build latest documentation

3eb7be8

Improve release notes and documentation

3be82b7

Address code review feedback.

dcalhoun force-pushed the rnmobile/set-and-reference-heading-anchors branch from 61a2168 to 3be82b7 Compare January 26, 2021 16:41

ceyhun merged commit 946d54b into WordPress:master Jan 26, 2021

github-actions bot added this to the Gutenberg 9.9 milestone Jan 26, 2021

dcalhoun deleted the rnmobile/set-and-reference-heading-anchors branch January 31, 2021 23:18

ntsekouras mentioned this pull request Mar 10, 2021

Fix React warning in Text Control #29724

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RNMobile] Set and reference heading anchors #27935

[RNMobile] Set and reference heading anchors #27935

dcalhoun commented Dec 30, 2020 •

edited

Loading

hypest commented Jan 8, 2021

dcalhoun commented Jan 13, 2021 •

edited

Loading

talldan commented Jan 14, 2021

ceyhun left a comment

hypest commented Jan 15, 2021

dcalhoun commented Jan 15, 2021

dcalhoun commented Jan 15, 2021

talldan left a comment

talldan Jan 20, 2021

dcalhoun Jan 20, 2021 •

edited

Loading

talldan Jan 20, 2021

dcalhoun Jan 20, 2021

hypest Jan 21, 2021

talldan Jan 20, 2021

dcalhoun Jan 20, 2021

hypest Jan 21, 2021 •

edited

Loading

dcalhoun Jan 21, 2021

ceyhun Jan 26, 2021

talldan Jan 20, 2021

dcalhoun commented Jan 20, 2021

ceyhun left a comment

ceyhun Jan 26, 2021

dcalhoun commented Jan 26, 2021

ceyhun commented Jan 26, 2021

[RNMobile] Set and reference heading anchors #27935

[RNMobile] Set and reference heading anchors #27935

Conversation

dcalhoun commented Dec 30, 2020 • edited Loading

Description

How has this been tested?

Screenshots

Types of changes

Checklist

hypest commented Jan 8, 2021

dcalhoun commented Jan 13, 2021 • edited Loading

talldan commented Jan 14, 2021

ceyhun left a comment

Choose a reason for hiding this comment

hypest commented Jan 15, 2021

dcalhoun commented Jan 15, 2021

dcalhoun commented Jan 15, 2021

talldan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcalhoun Jan 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hypest Jan 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcalhoun commented Jan 20, 2021

ceyhun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcalhoun commented Jan 26, 2021

ceyhun commented Jan 26, 2021

dcalhoun commented Dec 30, 2020 •

edited

Loading

dcalhoun commented Jan 13, 2021 •

edited

Loading

dcalhoun Jan 20, 2021 •

edited

Loading

hypest Jan 21, 2021 •

edited

Loading