Pinned items regression followup. #28526
Merged
Conversation
This file contains 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
jasmussen
added
[Type] Bug
|
A related issue for pinning/unpinning items that don't have menu items: #14457. |
|
Size Change: +96 B (0%) Total Size: 1.37 MB
ℹ️ View Unchanged
|
ntsekouras
approved these changes
Jan 27, 2021
|
Should we land this one as well, despite the failing e2e tests? Happy to merge if I get the thumbs up, and I'll follow up with a ticket for the subsequent improvement. |
Yes. The failing tests are the same from Core regression. |
@jasmussen At a quick look, the failing tests seem related to the media modal, which should be caused by a recent change in Core (see this comment), which was merged to the WP dev branch, which is what e2e tests run on. |
|
Thanks both. And of course I'll follow up with anything necessary, including that separate ticket. Dinner first, though 😅 |
This was referenced Jan 27, 2021
|
Ticketed the followup improvement in #28546. |
|
Cherry-picked to |
7 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Quick followup to #28521, apologies and thank you @Copons for input.
The previous fix hid all pinned items on mobile. But that as of recent changes, and possibly the original cause of the regression, that includes the settings sidebar.
This PR fixes that:
Jacopo further alerted me to the fact that it isn't necessarily as simple as a first-child solution, because if you unpin a plugin, then pin it again, it shows up at as the first child. However that doesn't survive a reload:
For that reason, I would suggest this PR is a good interim fix, because you can't even pin or unpin items on mobile at all.
But we should follow up with a separate fix that either moves the cog back out of the "pinned items" group, or makes sure it always shows up as the first child — or just that it has a child name that is easily targetted.