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.

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

Live Preview: Show the current theme name on the theme activation modal #57588

Conversation

arthur791004
Copy link
Contributor

@arthur791004 arthur791004 commented Jan 5, 2024

What?

Display the name of the currently active theme on the theme activation modal

Why?

Resolved Automattic/wp-calypso#81878

How?

Use the apiFetch with wp_theme_preview= to bypass the createThemePreviewMiddleware and get the currently active theme.

Another way is to create a new resolver, selector, and reducer to get the raw current theme and save it into the redux store. But I think it's only used by this modal now so we don't need to introduce new variable into the redux store.

Testing Instructions

  1. Go to /wp-admin/themes.php
  2. Preview a theme
  3. Click the “Activate” button
  4. Make sure the name of the currently active theme is shown in the description

Testing Instructions for Keyboard

Screenshots or screencast

image

@arthur791004 arthur791004 self-assigned this Jan 5, 2024
@arthur791004 arthur791004 removed request for mmtr and nerrad January 8, 2024 03:45
@arthur791004 arthur791004 force-pushed the feat/show-the-current-theme-on-the-theme-activation-modal branch from 22e7337 to 142d29d Compare January 8, 2024 03:48
} );

apiFetch( { path } ).then( ( activeThemes ) =>
setCurrentTheme( activeThemes[ 0 ] )
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it make sense to handle a request error here? Wondering if it's safe to access activeThemes[ 0 ] here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Handled the request error by 2f87b34.

Wondering if it's safe to access activeThemes[ 0 ] here.

It should be safe as we also access the value here.

Copy link
Contributor

@miksansegundo miksansegundo left a comment

Choose a reason for hiding this comment

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

LGTM 👍 It works as expected.

Screenshot 2567-01-08 at 19 11 39

@arthur791004 arthur791004 added the [Type] Enhancement A suggestion for improvement. label Jan 9, 2024
Copy link
Contributor

@okmttdhr okmttdhr left a comment

Choose a reason for hiding this comment

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

Looks good to me 🎉

packages/api-fetch/src/middlewares/theme-preview.js Outdated Show resolved Hide resolved
themeName
/* translators: %1$s: The name of active theme, %2$s: The name of theme to be activated. */
__(
'Saving your changes will change your active theme from %1$s to %2$s'
Copy link

Choose a reason for hiding this comment

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

We're missing the last period (.) here; it was there before this PR.

packages/edit-site/src/components/save-panel/index.js Outdated Show resolved Hide resolved
@arthur791004 arthur791004 merged commit 44ba13d into WordPress:trunk Jan 12, 2024
54 checks passed
@arthur791004 arthur791004 deleted the feat/show-the-current-theme-on-the-theme-activation-modal branch January 12, 2024 08:16
@github-actions github-actions bot added this to the Gutenberg 17.6 milestone Jan 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Live Preview: Shows the name of the current theme in the prompt text
4 participants