Skip to content

Feature/get redirects#22356

Merged
thijsoo merged 28 commits intofeature/redirection20from
feature/get-redirects
Jul 14, 2025
Merged

Feature/get redirects#22356
thijsoo merged 28 commits intofeature/redirection20from
feature/get-redirects

Conversation

@kyrylo-polozenko-newfold
Copy link
Copy Markdown
Contributor

@kyrylo-polozenko-newfold kyrylo-polozenko-newfold commented Jun 16, 2025

Context

This PR introduces full CRUD (Create, Read, Update, Delete) support for redirects, along with UI improvements and internal refactoring for better maintainability and performance.

Summary

changelog: enhancement
This PR can be summarized in the following changelog entry:

Adds functionality for creating, retrieving, updating, and deleting redirects.

Includes design and UI improvements to the redirects interface.

Relevant technical choices:

Implemented useRedirectFilters and useGetRedirects hooks to manage state and data fetching.

Refactored logic for sorting, filtering, and bulk actions (e.g., bulk delete).

Added loading indicators and success/error notifications.

Improved table and form UI for a better user experience.

UI changes

  • This PR changes the UI in the plugin. I have added the 'UI change' label to this PR.

Documentation

  • I have written documentation for this change.

@kyrylo-polozenko-newfold kyrylo-polozenko-newfold added UI change PRs that result in a change in the UI changelog: enhancement Needs to be included in the 'Enhancements' category in the changelog labels Jun 16, 2025
@coveralls
Copy link
Copy Markdown

coveralls commented Jun 16, 2025

Pull Request Test Coverage Report for Build 30c6b14cf72bf2df4c2ac8a578b09d5053a0b516

Details

  • 1 of 23 (4.35%) changed or added relevant lines in 9 files are covered.
  • 105 unchanged lines in 8 files lost coverage.
  • Overall coverage increased (+0.1%) to 53.548%

Changes Missing Coverage Covered Lines Changed/Added Lines %
admin/class-config.php 0 1 0.0%
admin/views/redirects.php 0 1 0.0%
packages/js/src/redirects/components/route-error-fallback.js 0 1 0.0%
packages/js/src/redirects/components/route-layout.js 0 1 0.0%
packages/js/src/redirects/routes/redirects.js 0 1 0.0%
packages/js/src/redirects/initialize.js 0 2 0.0%
packages/js/src/redirects/appProvider.js 0 5 0.0%
packages/js/src/redirects/store/preferences.js 0 10 0.0%
Files with Coverage Reduction New Missed Lines %
packages/js/src/shared-admin/hooks/use-formik-error.js 1 0.0%
packages/js/src/shared-admin/hooks/use-parsed-user-agent.js 1 0.0%
packages/js/src/redirects/constants/index.js 1 0.0%
packages/js/src/redirects/components/route-layout.js 1 0.0%
src/integrations/admin/redirects-page-integration.php 2 89.47%
packages/js/src/redirects/routes/redirects.js 2 0.0%
admin/class-config.php 23 0.0%
admin/views/redirects.php 74 0.0%
Totals Coverage Status
Change from base Build 9eca112a43d47f6670b05e1adc82fe6694c5958e: 0.1%
Covered Lines: 30345
Relevant Lines: 57698

💛 - Coveralls

@vraja-pro vraja-pro self-assigned this Jun 16, 2025
Comment thread css/src/new-redirects.css Outdated
Comment thread css/src/new-redirects.css Outdated
Comment thread css/src/new-redirects.css
Comment thread css/src/new-redirects.css Outdated
Comment thread css/src/new-redirects.css Outdated
Comment thread css/src/new-redirects.css Outdated
Comment thread css/src/new-redirects.css Outdated
margin-inline-start: 1.5rem;
}
.yst-text-field > div:first-child {
display: none !important;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why not use hidden?

Suggested change
display: none !important;
@apply yst-hidden;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

can't beat specificity, there is important on component

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why do you want to hide the first child here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I moved it a little under another class, this is needed for search input since we don't have a label and it comes with margin-bottom by default, I decided to hide this block since it is not needed it gives extra height

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If you don't need the label then you can use TextInput component from the ui-library and add a class to it for the search icon yst-ps-9 and you are good to go.

Comment thread css/src/new-redirects.css Outdated
Comment thread css/src/new-redirects.css Outdated
Comment thread css/src/new-redirects.css Outdated
Comment thread packages/js/src/redirects/components/filter-controls.js Outdated
Comment thread packages/js/src/redirects/components/notifications.js Outdated
Comment thread css/src/new-redirects.css Outdated

.yst-root {
.yst-notifications {
max-height: calc(100% - 4rem - 32px); /* Extra 32px for the height of the WP admin bar. */
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please check if you can use tailwind css for that https://tailwindcss.com/docs/max-height#examples

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I checked the documentation of tailwind and asked chatgpt directly, it is not possible to implement

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I would like to have a patch to test it. Lets see if we can find a cleaner way to address that.

Copy link
Copy Markdown
Contributor

@vraja-pro vraja-pro left a comment

Choose a reason for hiding this comment

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

CR 🚧
I left some comments for tailwind css, and documentation.
Also the lint tests are failing.

<ApacheSettings isApache={ settings?.is_apache } />
</div>
</FormLayout>
<Notifications />
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We don't need notifications in free right?

@@ -21,18 +21,21 @@ import {
const createStore = ( { initialState } ) => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe the store can also be created on premium

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor

@vraja-pro vraja-pro left a comment

Choose a reason for hiding this comment

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

Lets see if we can move more logic to premium and maybe add more slots or just pass more props.

Copy link
Copy Markdown
Contributor

@thijsoo thijsoo left a comment

Choose a reason for hiding this comment

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

Some comments inline about needing a different approach on some spots and more code needs to be moved. Let me know if everything is clear :)

I will leave some more in depth comments in the premium PR as well.

element={
<SidebarLayout>
<RedirectMethod />
<Redirects { ...redirectsProps } />
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We need to replace this with an construction where the free repo only contains a static upsell component with no logic. and the Premium repo overwrites this slot when its enabled with the actual component. This is also the case for the other components in this sidebar layout. Since they are not clickable they cannot be in the free repo.

Comment on lines +47 to +68
{ isPremium && (
<>
<Route
path={ ROUTES.regexRedirects }
element={
<SidebarLayout>
<RegexRedirects { ...regexProps } />
</SidebarLayout>
}
errorElement={ <RouteErrorFallback /> }
/>
<Route
path={ ROUTES.redirectMethod }
element={
<SidebarLayout>
<RedirectMethod />
</SidebarLayout>
}
errorElement={ <RouteErrorFallback /> }
/>
</>
) }
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This needs to be replaced by a that is filled from the premium repo.

<FilterControls />
</div>
</FormLayout>
<Formik
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I would like us to move this entirely to the premium repo and only have a static representation fo this page in the free repo as the upsell.

* @returns {JSX.Element} The rendered RegexRedirects route.
*/
export const RegexRedirects = () => {
export const RegexRedirects = ( {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This entire component needs to only live in the premium repo. This cannot be used in free so it cannot be here.

* @param {string} parentPath The parent object path.
* @returns {Object} Flattened object.
*/
export const flattenObject = ( object, parentPath = "" ) => reduce(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I am not sure this is used anywhere in the free repo currently? If its only used in the premium implementation it should be moved.

};


SidebarLayout.propTypes = {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is double.

className="yst-flex yst-gap-1.5"
>
<LockClosedIcon className="yst-w-4 yst-h-4" />
<span>Unlock with Premium</span>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is not translated.

{ value: "307", label: __( "307 Temporary Redirect", "wordpress-seo" ) },
{ value: "410", label: __( "410 Content Deleted", "wordpress-seo" ) },
{ value: "451", label: __( "451 Unavailable For Legal Reasons", "wordpress-seo" ) },
{ value: 301, label: __( "301 Moved Permatently", "wordpress-seo" ) },
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this file is not needed anymore since all actual features will move to premium?

@@ -0,0 +1,14 @@
/**
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This file can be removed/moved to premium since its not used in the free code.

@@ -1,2 +1 @@
export * from "./submit";
export * from "./validation";
export * from "./i18n";
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This file can be removed/moved to premium since its not used in the free code.

Comment thread css/src/new-redirects.css
@@ -61,6 +61,12 @@ body.seo_page_wpseo_redirects {
}

.yst-root {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We also need to not name things new this is just the redirects. No need for new.

* @returns {JSX.Element} The rendered Redirects route.
*/
export const Redirects = () => {
const redirectsManagedLink = useSelectRedirects( "selectLink", [], "https://yoast.com/yoast-seo-redirect-manager" );
Copy link
Copy Markdown
Contributor

@thijsoo thijsoo Jul 10, 2025

Choose a reason for hiding this comment

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

Suggested change
const redirectsManagedLink = useSelectRedirects( "selectLink", [], "https://yoast.com/yoast-seo-redirect-manager" );
const redirectsManagedLink = useSelectRedirects( "selectLink", [], "https://yoa.st/redirects-learn-more" );

Copy link
Copy Markdown
Contributor

@thijsoo thijsoo left a comment

Choose a reason for hiding this comment

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

CR + ACC 👍

@thijsoo thijsoo merged commit 5edee4b into feature/redirection20 Jul 14, 2025
55 checks passed
@thijsoo thijsoo deleted the feature/get-redirects branch July 14, 2025 11:22
@vraja-pro vraja-pro mentioned this pull request Jul 28, 2025
18 tasks
@enricobattocchi enricobattocchi added changelog: non-user-facing Needs to be included in the 'Non-userfacing' category in the changelog and removed changelog: enhancement Needs to be included in the 'Enhancements' category in the changelog labels Jul 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changelog: non-user-facing Needs to be included in the 'Non-userfacing' category in the changelog UI change PRs that result in a change in the UI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants