Skip to content

Use TagsView for "Site Settings -> Tags" #24674

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.

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

Draft
wants to merge 2 commits into
base: feature/tags-data-view-post-settings
Choose a base branch
from

Conversation

crazytonyli
Copy link
Contributor

Note

This PR is built on top of #24670.

Description

Following the existing design, but implemented with SwiftUI. The existing UI has an issue with loading a large number of tags, which is fixed in the new implementation (see https://linear.app/a8c/issue/CMM-96).

Old New
old.mp4
new.mp4

Testing instructions

@crazytonyli crazytonyli added this to the 26.1 milestone Jul 15, 2025
@crazytonyli crazytonyli requested a review from kean July 15, 2025 01:26
@dangermattic
Copy link
Collaborator

1 Warning
⚠️ This PR is larger than 500 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews.
1 Message
📖 This PR is still a Draft: some checks will be skipped.

Generated by 🚫 Danger

Copy link

if let total {
self.total = total + newItems.count
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

How should the Data View handle a new item is added from the client side? Like adding a new tag in this case.

We should not refresh the Data View, because the new item may not be on the first page.

We can insert the new item into the fetched items. But where? We can only try to mimic the server-side implementation, based on the order of response items. Then the DataViewPaginatedResponse needs to have a sorting function, which may complicate things.

In this PR, I went with the simplest solution, where the new item is placed at the top of the list.

Let me know what you think, @kean

Copy link
Contributor

Choose a reason for hiding this comment

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

Inserting at the top seems like the best option.

@wpmobilebot
Copy link
Contributor

App Icon📲 You can test the changes from this Pull Request in WordPress by scanning the QR code below to install the corresponding build.
App NameWordPress
ConfigurationRelease-Alpha
Build Number28191
VersionPR #24674
Bundle IDorg.wordpress.alpha
Commit218eb05
Installation URL0l4urakocdcr0
Automatticians: You can use our internal self-serve MC tool to give yourself access to those builds if needed.

@wpmobilebot
Copy link
Contributor

App Icon📲 You can test the changes from this Pull Request in Jetpack by scanning the QR code below to install the corresponding build.
App NameJetpack
ConfigurationRelease-Alpha
Build Number28191
VersionPR #24674
Bundle IDcom.jetpack.alpha
Commit218eb05
Installation URL0m320cqit15ig
Automatticians: You can use our internal self-serve MC tool to give yourself access to those builds if needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants