Update/unify link interfaces #6392 #7022
Conversation
|
@karmatosed I need to write tests for this PR but I'm unsure how to test the Popover component. Do you mind pointing me to a few resources? |
|
@RockinRonE let's see about getting you some help on that from @aduth or @youknowriad - thanks for testing! |
There was a problem hiding this comment.
Thanks for the contribution @RockinRonE !
Regarding the unit tests, can you explain a bit more what it is specifically about Popover behaviors being revised here that you're wanting to test?
Here are some general resources to help you get started with testing:
| @@ -47,6 +47,20 @@ const blockAttributes = { | |||
| source: 'attribute', | |||
| selector: 'figure > a', | |||
| attribute: 'href', | |||
| default: null, | |||
There was a problem hiding this comment.
Can you explain why we need the default value?
There was a problem hiding this comment.
It was because I kept receiving this error when running tests. I wasn't exactly sure what to do so please feel free to suggest and I'll look into it!
| import { IconButton } from '@wordpress/components'; | ||
| import { IconButton, ToggleControl, Popover } from '@wordpress/components'; | ||
| import { keycodes } from '@wordpress/utils'; | ||
| import { filterURLForDisplay } from '../../utils/url'; |
There was a problem hiding this comment.
Would you mind putting this into a separate dependencies section labelled "Internal dependencies" ?
See: https://github.com/WordPress/gutenberg/blob/master/docs/reference/coding-guidelines.md#imports
Notably, this would apply for any imports which are made to a relative path within the same top-level folder.
There was a problem hiding this comment.
Thank you for the link and my apologies
| } | ||
|
|
||
| setLinkTarget( opensInNewWindow ) { | ||
| this.setState( { opensInNewWindow } ); |
There was a problem hiding this comment.
Do we need to track this as part of state?
Or can it be inferred from props in render as:
const opensInNewWindow = ( this.props.attributes.target === '_blank' );The hope being that we could consolidate to one source of truth (attributes), rather than tracking in two places, which is more prone to falling out of sync.
Edit: I see after the fact that this is also how we populate the initial state in the constructor.
| const buttonLabel = url ? __( 'Edit Link' ) : __( 'Insert Link' ); | ||
|
|
||
| const linkSettings = settingsVisible && ( | ||
| <div className="editor-format-toolbar__link-modal-line editor-format-toolbar__link-settings"> |
There was a problem hiding this comment.
The coding guidelines for CSS naming is written such that this is not a valid class name to assign to this component. It's reflective of the fact that components are meant to compose one another, and shouldn't be inheriting behaviors, since it can lead to accidental breakage if a change to the format toolbar inadvertently affects this component. Isolating components to silo their behavior / silos helps prevent this.
What I'd like to see instead, and what seemed to be hinted by the idea of "unifying" in the original pull request, is that instead of duplicating what already exists elsewhere, we ought to consider creating a new, separate component which contains the shared behavior (of link management), then use that both here and in other existing usage.
| } | ||
| </div> | ||
| </div > |
There was a problem hiding this comment.
This change, while not explicitly part of the coding standards, does not seem necessary and is in contrast with style convention of JSX used elsewhere in the codebase.
|
This pull request appears to have languished and will not be easily reconciled with master. Please feel free to reopen and rebase against the current master, or open a new pull request. |
Description
I updated the linking form to match the current one. When you add a link to an image the form looks and behaves like when you would transform text to a link.
How has this been tested?
I need assistance on this. I am unsure how to go about testing a Popover despite my research.
Screenshots
When clicking on the link button
When link has been previously set
Added toggelable menu for setting link options
Types of changes
Updated the UrlInputButton to align with existing add link
Checklist:
Additional Notes
I'm pretty fresh to contributing to open source. I'd appreciate any help you can provide with my PR :)