Rewrite URL (Permalink) panel as a popover#42033
Merged
Merged
Conversation
noisysocks
added
[Type] Enhancement
Member
Author
|
I need to test some of the edge cases (post types that don't support editable permalinks, users that don't have permission) but should be good enough for some design feedback cc. @javierarce. One thing I don't like is that the button label only shows the start of the URL which is fairly useless. Maybe we can truncate it at the beginning or in the middle. |
7 tasks
|
Size Change: -32 B (0%) Total Size: 1.25 MB
ℹ️ View Unchanged
|
Member
That would be great. The We could probably remove the extra URL label when a site doesn't use pretty permalinks ( Screenshot
|
Mamaduka
reviewed
Jun 30, 2022
Mamaduka
reviewed
Jun 30, 2022
Mamaduka
reviewed
Jun 30, 2022
Mamaduka
approved these changes
Jun 30, 2022
Member
Mamaduka
left a comment
Mamaduka
left a comment
There was a problem hiding this comment.
The code looks good, and the refactors test well for me. I tested with built-in and custom post types with different settings combinations and couldn't spot any regressions.
Member
Author
|
OK, I made it so that long URLs simply wrap to a new line. I also started passing them through
I experimented with making them truncate on the left which you can see in 58bec17, but I don't like it as much. It's also technically cumbersome.
|
Member
Author
Good flag. In this case should we remove the popover and have the button link straight to the URL @javierarce? Or maybe it's okay how it is because this way the user can copy the URL. |
Contributor
Yes, I would keep using the popover because of that reason and because it could be confusing to have an item that behaves differently from the others, even if it included the external link icon inside. Something we could do in these cases is remove the label above the URL and just show the title and the link:
Or we could leave the label but apply this suggestion. Thanks for bringing this up, @Mamaduka. |
Contributor
I didn't truncate the string on the design, but maybe we should set a limit so the URL doesn't go beyond 3 or 4 lines. Also, would it be overkill to truncate in the middle and always respect the permalink? For example, if we had |
Member
Author
Done 👍 I tried both suggestions and the one with two labels looked a little odd to me.
I think this isn't necessary. Because we're removing the |
noisysocks
force-pushed
the
add/url-popover
branch
from
2206e86 to
8ddbcde
Compare
Member
Author
|
OK, this is ready for re-review! |
noisysocks
commented
Jul 1, 2022
| } from '@wordpress/e2e-test-utils'; | ||
|
|
||
| const permalinkPanelXPath = `//div[contains(@class, "edit-post-sidebar")]//button[contains(@class, "components-panel__body-toggle") and contains(text(),"Permalink")]`; | ||
| // TODO: Use a more accessible selector. |
Member
Author
There was a problem hiding this comment.
I'll fix this in a follow up PR. It points to how we need to set proper aria-labels on these buttons. Right now screen readers read them as Public, June 27, example.com/page, Default template which I can't imagine is very useful. It should be something like Select visibility: Public, Change publish date: June 27, Change URL: example.com/page, Change template: Default template.
Why a separate PR and not now? Because the issue already exists in trunk and I want proper a11y / copy feedback.
Member
There was a problem hiding this comment.
We can also migrate the e2e tests to Playwright while we are at it 😄 The locator works great with labels.
Member
|
Thanks for addressing the feedback, @noisysocks. I'm not sure if unit test failures are related, so you might want to look into that. Screenshots
|
Mamaduka
reviewed
Jul 1, 2022
|
|
||
| it( 'should not render when the modal is not active', () => { | ||
| useSelect.mockImplementation( () => ( { isModalActive: false } ) ); | ||
| useSelect.mockImplementation( () => false ); |
Member
There was a problem hiding this comment.
Select now just returns a boolean used for isModalActive.
Member
Author
|
Thanks for bringing this home @Mamaduka! |
24 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What?
Rewrites the Permalink panel to be a URL popover in the Summary (formerly Status & visibility) panel.
Why?
Part of #39082.
How?
PostLinkcomponent from@wordpress/edit-post.PostURLcomponent to@wordpress/edit-postand@wordpress/editor.PostScheduleandPostVisibility.PostLink.Testing Instructions
Screenshots or screencast
Kapture.2022-06-29.at.16.57.28.mp4