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

fix: Show permalink editor in editor #7494

Merged
merged 2 commits into from
Jun 26, 2018
Merged

fix: Show permalink editor in editor #7494

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3ac5db6
fix: Show permalink editor in editor (fix #7467)
tofumatt Jun 22, 2018
7f3afdc
Use getCurrentPost() to get the link
tofumatt Jun 25, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 14 additions & 5 deletions editor/components/post-permalink/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,11 +56,11 @@ class PostPermalink extends Component {
}

render() {
const { isNew, previewLink, isEditable, samplePermalink, isPublished } = this.props;
const { isNew, postLink, isEditable, samplePermalink, isPublished } = this.props;
const { isCopied, isEditingPermalink } = this.state;
const ariaLabel = isCopied ? __( 'Permalink copied' ) : __( 'Copy the permalink' );

if ( isNew || ! previewLink ) {
if ( isNew || ! postLink ) {
return null;
}

Expand All @@ -80,7 +80,7 @@ class PostPermalink extends Component {
{ ! isEditingPermalink &&
<Button
className="editor-post-permalink__link"
href={ ! isPublished ? previewLink : samplePermalink }
href={ ! isPublished ? postLink : samplePermalink }
target="_blank"
ref={ ( permalinkButton ) => this.permalinkButton = permalinkButton }
>
Expand Down Expand Up @@ -123,10 +123,19 @@ class PostPermalink extends Component {

export default compose( [
withSelect( ( select ) => {
const { isEditedPostNew, isPermalinkEditable, getEditedPostPreviewLink, getPermalink, isCurrentPostPublished } = select( 'core/editor' );
const {
isEditedPostNew,
Copy link
Member Author

Choose a reason for hiding this comment

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

I was messing about with possibly improving how the logic to display the permalink editor worked and it meant editing the selectors used… I didn't end up doing it, but I found this a bit easier to read so left it in 😄

isPermalinkEditable,
getCurrentPost,
getPermalink,
isCurrentPostPublished,
} = select( 'core/editor' );

const { link } = getCurrentPost();

return {
isNew: isEditedPostNew(),
previewLink: getEditedPostPreviewLink(),
postLink: link,
isEditable: isPermalinkEditable(),
samplePermalink: getPermalink(),
isPublished: isCurrentPostPublished(),
Expand Down
11 changes: 0 additions & 11 deletions editor/store/selectors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -444,17 +444,6 @@ export function getDocumentTitle( state ) {
return title;
}

/**
* Returns a URL to preview the post being edited.
*
* @param {Object} state Global application state.
*
* @return {string} Preview URL.
*/
export function getEditedPostPreviewLink( state ) {
Copy link
Member

Choose a reason for hiding this comment

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

Should we deprecate this? On the one hand, it's a public 'method'. On the other, it's undocumented.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not documented, nothing else uses it, and @aduth mentioned it just missed the cutting block during another commit. I think technically maybe we should but I feel like we're in the clear.

I will add it back, deprecate it, and wear the dunce hat if it affects anyone in the next release 😉

Copy link
Member

Choose a reason for hiding this comment

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

Worth noting: It doesn't work as-is anyways, hence the original issue. Probably better to not work than to throw errors though. If we wanted full compatibility, we might redirect the result to getAutosaveAttribute( 'preview_link' )

Copy link
Member Author

Choose a reason for hiding this comment

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

But getAutosaveAttribute( 'preview_link' ) won't work on page load, right? I tested this with new page loads of existing posts and "Add New" posts and it never broke the page 🤷‍♂️

return getCurrentPost( state ).preview_link || null;
}

/**
* Returns a new reference when the inner blocks of a given block UID change.
* This is used exclusively as a memoized selector dependant, relying on this
Expand Down
21 changes: 0 additions & 21 deletions editor/store/test/selectors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@ const {
hasAutosave,
isEditedPostEmpty,
isEditedPostBeingScheduled,
getEditedPostPreviewLink,
getBlockDependantsCacheBust,
getBlockName,
getBlock,
Expand Down Expand Up @@ -1314,26 +1313,6 @@ describe( 'selectors', () => {
} );
} );

describe( 'getEditedPostPreviewLink', () => {
it( 'should return null if the post has not link yet', () => {
const state = {
currentPost: {},
};

expect( getEditedPostPreviewLink( state ) ).toBeNull();
} );

it( 'should return the correct url when the post object has a preview_link', () => {
const state = {
currentPost: {
preview_link: 'https://andalouses.com/?p=1&preview=true',
},
};

expect( getEditedPostPreviewLink( state ) ).toBe( 'https://andalouses.com/?p=1&preview=true' );
} );
} );

describe( 'getBlockDependantsCacheBust', () => {
const rootBlock = { uid: 123, name: 'core/paragraph', attributes: {} };
const rootOrder = [ 123 ];
Expand Down