Removed i18n toggle from labs UI (#21927) #21975
Conversation
ErisDS
force-pushed
the
i18n-flag-again
branch
from
7ec6922 to
6c4bbb4
Compare
WalkthroughThe changes involve removing the internationalization (i18n) feature toggle from the admin settings and updating a related end-to-end test file. The modification simplifies the labs settings by eliminating the portal translation feature toggle, while simultaneously refactoring the language-setting test logic into a more modular and reusable function. Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (11)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
ghost/core/test/e2e-browser/admin/i18n.spec.js (2)
5-12: Good extraction of reusable function, consider adding error handling and documentation.The function follows good practices with proper selector usage and clear responsibility. Consider these improvements:
- Add JSDoc documentation for better reusability
- Add error handling for invalid languages
- Verify successful language change
+/** + * Sets the publication language in Ghost admin settings + * @param {import('@playwright/test').Page} sharedPage - The shared page object + * @param {string} language - The language code (e.g., 'en', 'fr') + * @throws {Error} If language change fails + */ async function setLanguage(sharedPage, language) { await sharedPage.goto('/ghost/#/settings/publication-language'); const section = sharedPage.getByTestId('publication-language'); await section.getByRole('button', {name: 'Edit'}).click(); const input = section.getByPlaceholder('Site language'); await input.fill(language); await section.getByRole('button', {name: 'Save'}).click(); + // Verify language was set + await section.getByText(language, {exact: true}).waitFor({state: 'visible'}); }
39-40: Consider using afterEach for language reset.Good practice resetting the language to prevent test interference! Consider moving this to an afterEach hook for better test structure and automatic cleanup:
test.describe('i18n', () => { + test.afterEach(async ({sharedPage}) => { + await setLanguage(sharedPage, 'en'); + }); + test.describe('Newsletter', () => { test('changing the site language immediately translates strings in newsletters', async ({sharedPage}) => { await setLanguage(sharedPage, 'fr'); // ... test code ... - // Set the language back before the next test! - await setLanguage(sharedPage, 'en'); }); }); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/admin-x-settings/src/components/settings/advanced/labs/BetaFeatures.tsx(0 hunks)ghost/core/test/e2e-browser/admin/i18n.spec.js(2 hunks)
💤 Files with no reviewable changes (1)
- apps/admin-x-settings/src/components/settings/advanced/labs/BetaFeatures.tsx
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Browser tests
- GitHub Check: Ghost-CLI tests
- GitHub Check: Database tests (Node 20.11.1, sqlite3)
- GitHub Check: Database tests (Node 20.11.1, mysql8)
- GitHub Check: Regression tests (Node 20.11.1, sqlite3)
- GitHub Check: Database tests (Node 18.12.1, mysql8)
- GitHub Check: Regression tests (Node 20.11.1, mysql8)
- GitHub Check: Admin-X Settings tests
🔇 Additional comments (1)
ghost/core/test/e2e-browser/admin/i18n.spec.js (1)
Line range hint
17-38: Well-structured test with clear assertions!The test properly sets up the French language environment, performs the newsletter operations, and validates the translations with explicit expectations.
ref bb9a69e ref https://linear.app/ghost/issue/ENG-1753/labs-flags-cleanup - We promoted i18n to GA several weeks ago now, and it's going fine - Removing the UI first to reduce confusion before cleaning up all the other references to the flag - Also changed the i18n test to set the language back at the end of the test, to ensure no conflicts
ErisDS
force-pushed
the
i18n-flag-again
branch
from
6c4bbb4 to
52020ef
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
ghost/core/test/e2e-browser/admin/i18n.spec.js (3)
5-11: Good extraction of reusable functionality, consider adding error handling.The function follows good practices by using test IDs and maintaining consistent async/await patterns. However, it could be more robust.
Consider these improvements:
async function setLanguage(sharedPage, language) { + if (!language || typeof language !== 'string') { + throw new Error('Language parameter must be a non-empty string'); + } await sharedPage.goto('/ghost/#/settings/publication-language'); const section = sharedPage.getByTestId('publication-language'); const input = section.getByPlaceholder('Site language'); await input.fill(language); await section.getByRole('button', {name: 'Save'}).click(); + // Verify save was successful + await section.getByText('Saved').waitFor(); }
38-39: Enhance the cleanup comment to be more descriptive.The current comment could better explain the importance of the language reset.
- // Set the language back before the next test! + // Reset language to English to maintain consistent test environment and prevent side effects in subsequent tests await setLanguage(sharedPage, 'en');
16-16: Consider verifying the language reset.While the test properly resets the language, it doesn't verify that the reset was successful.
Consider adding an assertion after the reset:
await setLanguage(sharedPage, 'en'); + // Verify reset was successful + await sharedPage.goto('/ghost'); + const metaTextAfterReset = await sharedPage.frameLocator('iframe.gh-pe-iframe').locator('td.post-meta').first().textContent(); + expect(metaTextAfterReset).toContain('By Joe Bloggs'); + expect(metaTextAfterReset).not.toContain('Par Joe Bloggs');Also applies to: 39-39
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/admin-x-settings/src/components/settings/advanced/labs/BetaFeatures.tsx(0 hunks)ghost/core/test/e2e-browser/admin/i18n.spec.js(2 hunks)
💤 Files with no reviewable changes (1)
- apps/admin-x-settings/src/components/settings/advanced/labs/BetaFeatures.tsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Browser tests
Trying again! 🙈
ref bb9a69e
ref https://linear.app/ghost/issue/ENG-1753/labs-flags-cleanup
Summary by CodeRabbit
Removed Feature
Testing