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

fix: string translation #2401

Merged
merged 5 commits into from
Oct 23, 2024
Merged

Conversation

Soare-Robert-Daniel
Copy link
Contributor

@Soare-Robert-Daniel Soare-Robert-Daniel commented Oct 7, 2024

Closes https://github.com/Codeinwp/otter-internals/issues/232

Summary

Fixed the issues related to translation and some errors reported by Eslint.

Note

I found that the eEslint config file was not working with wp presets. Thus, after fixing it, there are some extra fixes in this PR based on WordPress rules. Some were marked as warnings since they are not a priority for now.

The generate .pot file with ALL STRINGS: https://pastebin.com/cKzTCthH

Archive with all strings and separated one with only pro strings: Archive.zip (generated with wp i18n make-pot . languages/otter-all-strings.pot and wp i18n make-pot . languages/otter-pro-strings.pot --include=src/pro,otter-pro )

Testing

Check if all the necessary strings are in the pot file.


Checklist before the final review

  • Included E2E or unit tests for the changes in this PR.
  • Visual elements are not affected by independent changes.
  • It is at least compatible with the minimum WordPress version.
  • It loads additional script in frontend only if it is required.
  • Does not impact the Core Web Vitals.
  • In case of deprecation, old blocks are safely migrated.
  • It is usable in Widgets and FSE.
  • Copy/Paste is working if the attributes are modified.
  • PR is following the best practices

@Soare-Robert-Daniel Soare-Robert-Daniel self-assigned this Oct 7, 2024
@Soare-Robert-Daniel Soare-Robert-Daniel linked an issue Oct 7, 2024 that may be closed by this pull request
@Soare-Robert-Daniel Soare-Robert-Daniel added the pr-checklist-skip Allow this Pull Request to skip checklist. label Oct 9, 2024
@Soare-Robert-Daniel Soare-Robert-Daniel marked this pull request as ready for review October 9, 2024 12:24
@pirate-bot pirate-bot added the pr-checklist-complete The Pull Request checklist is complete. (automatic label) label Oct 9, 2024
@pirate-bot
Copy link
Contributor

pirate-bot commented Oct 9, 2024

Bundle Size Diff

Package Old Size New Size Diff
Animations 271.38 KB 271.41 KB 27 B (0.01%)
Blocks 1.56 MB 1.56 MB 510 B (0.03%)
CSS 100.87 KB 100.89 KB 20 B (0.02%)
Dashboard 198.32 KB 198.45 KB 127 B (0.06%)
Onboarding 161.11 KB 161.1 KB -14 B (-0.01%)
Export Import 97.78 KB 97.78 KB 0 B (0.00%)
Pro 422.93 KB 423.27 KB 350 B (0.08%)

@pirate-bot
Copy link
Contributor

pirate-bot commented Oct 9, 2024

Plugin build for 9481240 is ready 🛎️!

@Soare-Robert-Daniel Soare-Robert-Daniel force-pushed the fix/string-translation branch 2 times, most recently from e3418e1 to 8a70889 Compare October 16, 2024 07:46
@pirate-bot
Copy link
Contributor

pirate-bot commented Oct 17, 2024

E2E Tests

Playwright Test Status:

Performance Results serverResponse: 237.65, firstPaint: 555.55, domContentLoaded: 1531.45, loaded: 1532, firstContentfulPaint: 3439.75, firstBlock: 7652, type: 14.32, minType: 13.07, maxType: 16.25, typeContainer: 9.4, minTypeContainer: 7.82, maxTypeContainer: 11.83, focus: 32.21, minFocus: 30.1, maxFocus: 38.75, inserterOpen: 22.15, minInserterOpen: 19.44, maxInserterOpen: 25.25, inserterSearch: 0.82, minInserterSearch: 0.76, maxInserterSearch: 0.9, inserterHover: 3.1, minInserterHover: 2.58, maxInserterHover: 4.51, listViewOpen: 145.66, minListViewOpen: 132.34, maxListViewOpen: 159.51

@@ -161,7 +161,7 @@ const deprecated = [{
'wp-block-themeisle-blocks-button',
`wp-block-themeisle-blocks-button-${ i }`
) }
style={ buttonStyle }
style={ buttonStyle } rel="noreferrer"
Copy link
Member

Choose a reason for hiding this comment

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

We don't want to modify deprecated blocks. This might cause errors with users updating from old HTML structure.

@@ -651,7 +651,7 @@ const Edit = ({
} else {
createNotice(
'error',
__( 'An error has occurred: ', 'otter-blocks' ) + ( res?.error || __( 'unknown', 'otter-blocks' ) ),
__( 'An error has occurred:', 'otter-blocks' ) + ( res?.error || __( 'unknown', 'otter-blocks' ) ),
Copy link
Member

Choose a reason for hiding this comment

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

The space there is intentional so the error message has a space between the previous sentence. Either we can add it back or switch to using sprintf instead.

@@ -700,7 +700,7 @@ const Edit = ({
} else {
createNotice(
'error',
__( 'An error has occurred: ', 'otter-blocks' ) + ( res?.error || __( 'unknown', 'otter-blocks' ) + __( '. Check your provider for confirmation.', 'otter-blocks' ) ),
__( 'An error has occurred:', 'otter-blocks' ) + ( res?.error || __( 'unknown', 'otter-blocks' ) + __( '. Check your provider for confirmation.', 'otter-blocks' ) ),
Copy link
Member

Choose a reason for hiding this comment

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

Same as before.

@@ -19,7 +19,7 @@ async function makeSearchRequest( location ) {
return response.json();
}

return console.warn( __( 'An error has occured: ', 'otter-blocks' ) + response.status );
return console.warn( __( 'An error has occured:', 'otter-blocks' ) + response.status );
Copy link
Member

Choose a reason for hiding this comment

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

This will also be better served with sprintf.

@@ -157,7 +157,7 @@ const Edit = ({
});

default:
console.warn( __( 'The action for the leaflet block do not have a defined action in marker\'s reducer: ', 'otter-blocks' ) + action.type );
console.warn( __( 'The action for the leaflet block do not have a defined action in marker\'s reducer:', 'otter-blocks' ) + action.type );
Copy link
Member

Choose a reason for hiding this comment

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

This will also be better served with sprintf.

@@ -36,7 +36,7 @@ const Save = ({
<div className="otter-popup__modal_content">
{ attributes.showClose && (
<div className="otter-popup__modal_header">
<button type="button" class="components-button has-icon"><svg width="24" height="24" xmlns="http://www.w3.org/2000/svg" viewBox="0 0 24 24" role="img" aria-hidden="true"><path d="M12 13.06l3.712 3.713 1.061-1.06L13.061 12l3.712-3.712-1.06-1.06L12 10.938 8.288 7.227l-1.061 1.06L10.939 12l-3.712 3.712 1.06 1.061L12 13.061z"></path></svg></button>
<button type="button" className="components-button has-icon"><svg width="24" height="24" xmlns="http://www.w3.org/2000/svg" viewBox="0 0 24 24" role="img" aria-hidden="true"><path d="M12 13.06l3.712 3.713 1.061-1.06L13.061 12l3.712-3.712-1.06-1.06L12 10.938 8.288 7.227l-1.061 1.06L10.939 12l-3.712 3.712 1.06 1.061L12 13.061z"></path></svg></button>
Copy link
Member

Choose a reason for hiding this comment

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

This is frontend HTML, it should not be using className but class as attribute.

Copy link
Member

Choose a reason for hiding this comment

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

Upon reviewing this more, I think this is fine. I'll just double check this to confirm it doesn't cause any invalidation with the change in save.js.

@@ -176,7 +176,7 @@ export const PostsMeta = ({ attributes, element, post, author, categories }) =>
if ( attributes.displayAuthor && undefined !== author ) {

/* translators: %s Author of the post */
postedOn += sprintf( __( ' by %s', 'otter-blocks' ), author.name );
postedOn += sprintf( __( 'by %s', 'otter-blocks' ), author.name );
Copy link
Member

Choose a reason for hiding this comment

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

We use space here to avoid there not being space between this and the previous element.

Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't be updating save methods. It can cause invalidation.

@@ -199,7 +198,7 @@ const Edit = ({

const addTab = () => {
const itemBlock = createBlock( 'themeisle-blocks/tabs-item', {
title: __( 'Tab ', 'otter-blocks' ) + ( ( children?.length ?? 0 ) + 1 )
title: __( 'Tab', 'otter-blocks' ) + ( ( children?.length ?? 0 ) + 1 )
Copy link
Member

Choose a reason for hiding this comment

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

This will also be better served with sprintf. We want there to be a space in between.

Copy link
Member

Choose a reason for hiding this comment

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

We don't want to update the save.js file + this is not valid HTML.

Copy link
Member

Choose a reason for hiding this comment

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

Same as before, here I will just confirm to make sure this doesn't cause any invalidation with changing of save.js file.

@@ -299,7 +296,7 @@ const TypographySelectorControl = ( props: TypographySelectorControlProps ) => {
{ defaultStates.componentNames?.[component] }
</MenuItem>;
}
return <Fragment></Fragment>;
return <Fragment key={index}></Fragment>;
Copy link
Member

Choose a reason for hiding this comment

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

We can just replace null here.

Comment on lines 225 to 235
if ( ! keys.includes( keyEvent.key ) ) {
return;
}

if ( 'Escape' === keyEvent.key ) {
inputElement.blur();
resultsContainer = removeResultsContainer( block, resultsContainer );
return;
}

Copy link
Member

Choose a reason for hiding this comment

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

We can remove these empty tabs.

@@ -126,7 +126,7 @@ const Edit = ({

<div { ...blockProps }>
{ 'image' === attributes.library && isURL ? (
<img src={ attributes.icon } />
<img src={ attributes.icon } alt="" />
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't make much sense to have empty alt text.

@@ -100,7 +100,7 @@ const Edit = ({
const parentClientId = select( 'core/block-editor' ).getBlockParents( clientId ).at( -1 );
const parentBlock = select( 'core/block-editor' ).getBlock( parentClientId );

setAttributes({ content: __( 'List item ', 'otter-blocks' ) + parentBlock.innerBlocks.length });
setAttributes({ content: __( 'List item', 'otter-blocks' ) + parentBlock.innerBlocks.length });
Copy link
Member

Choose a reason for hiding this comment

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

We can use sprintf here to preserve the spacing.

setResult( resultHistory?.[ resultHistoryIndex ].result );
setTokenUsageDescription( __( 'Used tokens: ', 'otter-blocks' ) + resultHistory[ resultHistoryIndex ].meta.usedToken );

setTokenUsageDescription( __( 'Used tokens:', 'otter-blocks' ) + resultHistory[ resultHistoryIndex ].meta.usedToken );
Copy link
Member

Choose a reason for hiding this comment

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

We can use sprintf here to preserve the spacing.

Comment on lines 103 to 108
const modal = activeFrame.querySelector( '.media-frame-content' );
if ( ! modal ) {
return false;
}

Copy link
Member

Choose a reason for hiding this comment

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

We can remove empty tabs.

@@ -62,7 +62,7 @@ const edit = props => {
<h3>{ __( 'There are 30+ more patterns and full page designs available in Otter PRO.', 'otter-blocks' ) }</h3>

<div className="o-block-patterns-upsell__actions">
<a href={ setUtm( window.themeisleGutenberg.patternsLink, 'patterns', 'otter-blockspatternslibrary' ) } target="_blank">
<a href={ setUtm( window.themeisleGutenberg.patternsLink, 'patterns', 'otter-blockspatternslibrary' ) } target="_blank" rel="noreferrer">
Copy link
Member

Choose a reason for hiding this comment

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

We don't want to hide referrer here. It will just be bad for our own analytics.

const isTypingNow = isTyping();

Copy link
Member

Choose a reason for hiding this comment

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

We can remove this extra spacing.

@@ -99,7 +103,7 @@ const Main = ({
<NoticeCard
slug="feedback"
>
<img src={ window.otterObj.assetsPath + 'images/dashboard-feedback.png' } style={ { maxWidth: '100%', objectFit: 'cover' } }/>
<img src={ window.otterObj.assetsPath + 'images/dashboard-feedback.png' } style={ { maxWidth: '100%', objectFit: 'cover' } } alt="" />
Copy link
Member

Choose a reason for hiding this comment

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

We can remove the empty alt or add some alt text there.

Copy link
Member

Choose a reason for hiding this comment

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

We can remove the empty alt or add some alt text there.

@@ -44,7 +44,7 @@ const Edit = ({
className: 'wp-block wp-block-themeisle-blocks-form-input'
});

const placeholder = attributes.paramName ? __( 'Get the value of the URL param: ', 'otter-blocks' ) + attributes.paramName : '';
const placeholder = attributes.paramName ? __( 'Get the value of the URL param:', 'otter-blocks' ) + attributes.paramName : '';
Copy link
Member

Choose a reason for hiding this comment

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

We can use sprintf here to preserve the space.

@@ -94,7 +94,7 @@ const WebhookEditor = ( props: WebhookEditorProps ) => {
for ( const webhook of webhooksToSave ) {
const check = checkWebhook( webhook );
if ( true !== check ) {
const msg = __( 'There was an error saving the webhook: ', 'otter-blocks' ) + webhook?.name + '\n';
const msg = __( 'There was an error saving the webhook:', 'otter-blocks' ) + webhook?.name + '\n';
Copy link
Member

Choose a reason for hiding this comment

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

We can use sprintf here to preserve the space.

Copy link
Member

@HardeepAsrani HardeepAsrani left a comment

Choose a reason for hiding this comment

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

Good job, this is a chonky boy. I've left comments with some quality of life improvements and some about changes that can cause invalidation, specially relating to all the save.js and deprecation files.

Apart from that, if you can do it quickly, it might be worth to replace all the instances of <Fragment></Fragment> with the <></> shorthand.

/* translators: %s Author of the post */
postedOn += sprintf( __( 'by %s', 'otter-blocks' ), author.name );
/* translators: %1$s is the date, %2$s is the author of the post */
postedOn = sprintf( __( '%1$s by %2$s', 'otter-blocks' ), postedOn, author.name );
Copy link
Member

Choose a reason for hiding this comment

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

If postedOn is empty until now, we will have an empty leading space. We can remove any leading/trailing spaces on postedOn variable.

Copy link
Member

@HardeepAsrani HardeepAsrani 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, left one small comment.

@Soare-Robert-Daniel Soare-Robert-Daniel merged commit 17379f8 into development Oct 23, 2024
11 checks passed
@Soare-Robert-Daniel Soare-Robert-Daniel deleted the fix/string-translation branch October 23, 2024 13:27
@pirate-bot
Copy link
Contributor

🎉 This PR is included in version 3.0.5 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@pirate-bot pirate-bot added the released Indicate that an issue has been resolved and released in a particular version of the product. label Oct 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-checklist-complete The Pull Request checklist is complete. (automatic label) pr-checklist-skip Allow this Pull Request to skip checklist. released Indicate that an issue has been resolved and released in a particular version of the product.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Prepare the plugin for localization
3 participants