Added URL input handling on CTA Card #1417
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ref https://linear.app/ghost/issue/PLG-313/implement-button-url-update - Added URL Input handling for the new CTA Card - Allows URL input or selecting from a populated list - Added additional test to ensure the button persists in the UI when conditions are met, which are whether the URL field is populated, Text is populated and the button is active.
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
packages/koenig-lexical/test/e2e/cards/button-card.test.jsOops! Something went wrong! :( ESLint: 8.57.0 ESLint couldn't find the config "react-app" to extend from. Please check that the name of the config is correct. The config "react-app" was referenced from the config file in "/packages/koenig-lexical/.eslintrc.cjs". If you still have problems, please stop by https://eslint.org/chat/help to chat with the team. WalkthroughThe pull request introduces enhancements to the Call To Action (CTA) card functionality in the Koenig Lexical editor. The changes focus on improving button URL handling, adding a data attribute for testing, and implementing a new method to dynamically update the button URL. Modifications were made to the component implementation and end-to-end testing, with new test cases verifying the button URL setting, suggestion, and interaction behaviors. Changes
Sequence DiagramsequenceDiagram
participant User
participant CtaCard
participant CallToActionNode
User->>CtaCard: Enter/Update Button URL
CtaCard->>CallToActionNode: updateButtonUrl(newUrl)
CallToActionNode-->>CtaCard: URL Updated
CtaCard->>CtaCard: Render Button with New URL
Possibly related PRs
Poem
✨ Finishing Touches
🪧 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: 1
🧹 Nitpick comments (2)
packages/koenig-lexical/test/e2e/cards/cta-card.test.js (2)
67-75: Consider adding edge cases to URL validation test.While the basic URL setting test is good, consider adding test cases for:
- Invalid URLs
- Empty URLs
- URLs with special characters
102-129: Improve test structure for better readability.The test covers multiple scenarios but could be more clearly structured. Consider breaking it into distinct sections:
- Setup (button settings, text, URL)
- Focus management
- State verification
test('button doesnt disappear when toggled, has text, has url and loses focus', async function () { + // Setup await focusEditor(page); await insertCard(page, {cardName: 'call-to-action'}); await page.click('[data-testid="button-settings"]'); await page.fill('[data-testid="button-text"]', 'Click me'); await page.fill('[data-testid="button-url"]', 'https://example.com/somepost'); + // Initial state verification expect(await page.isVisible('[data-testid="cta-button"]')).toBe(true); expect(await page.textContent('[data-testid="cta-button"]')).toBe('Click me'); const buttonContainer = await page.$('[data-test-cta-button-current-url]'); const currentUrl = await buttonContainer.getAttribute('data-test-cta-button-current-url'); expect(currentUrl).toBe('https://example.com/somepost'); + // Focus management await page.keyboard.press('Escape'); await page.keyboard.press('Enter'); + // Final state verification await assertHTML(page, html`...`); expect(await page.isVisible('[data-testid="cta-button"]')).toBe(true); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/koenig-lexical/src/components/ui/cards/CtaCard.jsx(1 hunks)packages/koenig-lexical/src/nodes/CallToActionNodeComponent.jsx(2 hunks)packages/koenig-lexical/test/e2e/cards/cta-card.test.js(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/koenig-lexical/src/components/ui/cards/CtaCard.jsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Node 20.11.1
🔇 Additional comments (1)
packages/koenig-lexical/src/nodes/CallToActionNodeComponent.jsx (1)
50-55: LGTM! Clean implementation of URL handling.The
handleButtonUrlChangeimplementation follows the established patterns in the codebase and correctly integrates with Lexical's editor API.Also applies to: 78-78
Comment on lines
77
to
100
| // NOTE: an improvement would be to pass in suggested url options, but the construction now doesn't make that straightforward | ||
| test('suggested urls display', async function () { | ||
| await focusEditor(page); | ||
| await insertCard(page, {cardName: 'call-to-action'}); | ||
| await page.click('[data-testid="button-settings"]'); | ||
|
|
||
| const buttonTextInput = await page.getByTestId('button-url'); | ||
| await expect(buttonTextInput).toHaveValue(''); | ||
|
|
||
| // this is dependent on the test values inserted in the node | ||
| await page.getByTestId('button-url').fill('Ho'); | ||
| // // this is too fast, need to try two inputs or add a delay before checking the suggested options | ||
| await page.getByTestId('button-url').fill('me'); | ||
| await expect(await page.getByTestId('button-url-listOption')).toContainText('Homepage'); | ||
| // click on homepage | ||
| await page.getByTestId('button-url-listOption').click(); | ||
| // check if the url is set | ||
| const buttonContainer = await page.$('[data-test-cta-button-current-url]'); | ||
| const currentUrl = await buttonContainer.getAttribute('data-test-cta-button-current-url'); | ||
| // current view can be any url, so check for a valid url | ||
| const validUrlRegex = /^(https?:\/\/)([\w.-]+)(:[0-9]+)?(\/[\w.-]*)*(\?.*)?(#.*)?$/; | ||
| // Assert the URL is valid | ||
| expect(currentUrl).toMatch(validUrlRegex); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Address potential flakiness in URL suggestions test.
A few concerns with the current implementation:
- The comment about timing suggests potential test flakiness
- Test relies on hardcoded test data ("Ho", "me", "Homepage")
- URL validation regex might be too permissive
Consider these improvements:
- Use
waitForor similar mechanism instead of relying on multiple input events - Inject test data through test configuration
- Use a stricter URL validation regex or a dedicated URL validation library
- // this is too fast, need to try two inputs or add a delay before checking the suggested options
- await page.getByTestId('button-url').fill('Ho');
- await page.getByTestId('button-url').fill('me');
+ await page.getByTestId('button-url').fill('Home');
+ await page.waitForSelector('[data-testid="button-url-listOption"]');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // NOTE: an improvement would be to pass in suggested url options, but the construction now doesn't make that straightforward | |
| test('suggested urls display', async function () { | |
| await focusEditor(page); | |
| await insertCard(page, {cardName: 'call-to-action'}); | |
| await page.click('[data-testid="button-settings"]'); | |
| const buttonTextInput = await page.getByTestId('button-url'); | |
| await expect(buttonTextInput).toHaveValue(''); | |
| // this is dependent on the test values inserted in the node | |
| await page.getByTestId('button-url').fill('Ho'); | |
| // // this is too fast, need to try two inputs or add a delay before checking the suggested options | |
| await page.getByTestId('button-url').fill('me'); | |
| await expect(await page.getByTestId('button-url-listOption')).toContainText('Homepage'); | |
| // click on homepage | |
| await page.getByTestId('button-url-listOption').click(); | |
| // check if the url is set | |
| const buttonContainer = await page.$('[data-test-cta-button-current-url]'); | |
| const currentUrl = await buttonContainer.getAttribute('data-test-cta-button-current-url'); | |
| // current view can be any url, so check for a valid url | |
| const validUrlRegex = /^(https?:\/\/)([\w.-]+)(:[0-9]+)?(\/[\w.-]*)*(\?.*)?(#.*)?$/; | |
| // Assert the URL is valid | |
| expect(currentUrl).toMatch(validUrlRegex); | |
| }); | |
| // NOTE: an improvement would be to pass in suggested url options, but the construction now doesn't make that straightforward | |
| test('suggested urls display', async function () { | |
| await focusEditor(page); | |
| await insertCard(page, {cardName: 'call-to-action'}); | |
| await page.click('[data-testid="button-settings"]'); | |
| const buttonTextInput = await page.getByTestId('button-url'); | |
| await expect(buttonTextInput).toHaveValue(''); | |
| await page.getByTestId('button-url').fill('Home'); | |
| await page.waitForSelector('[data-testid="button-url-listOption"]'); | |
| await expect(await page.getByTestId('button-url-listOption')).toContainText('Homepage'); | |
| // click on homepage | |
| await page.getByTestId('button-url-listOption').click(); | |
| // check if the url is set | |
| const buttonContainer = await page.$('[data-test-cta-button-current-url]'); | |
| const currentUrl = await buttonContainer.getAttribute('data-test-cta-button-current-url'); | |
| // current view can be any url, so check for a valid url | |
| const validUrlRegex = /^(https?:\/\/)([\w.-]+)(:[0-9]+)?(\/[\w.-]*)*(\?.*)?(#.*)?$/; | |
| // Assert the URL is valid | |
| expect(currentUrl).toMatch(validUrlRegex); | |
| }); |
There was a problem hiding this comment.
Fixed it, thanks.
There was a problem hiding this comment.
@ronaldlangeveld You're welcome! The test looks much more robust now. 🎉
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/koenig-lexical/test/e2e/cards/cta-card.test.js (3)
67-75: Consider adding edge cases for URL validation.The test verifies basic URL setting functionality well. Consider adding test cases for:
- Invalid URLs
- URLs with special characters
- Empty URL handling
test('handles invalid button urls', async function () { await focusEditor(page); await insertCard(page, {cardName: 'call-to-action'}); await page.click('[data-testid="button-settings"]'); // Test invalid URL await page.fill('[data-testid="button-url"]', 'not-a-url'); const buttonContainer = await page.$('[data-test-cta-button-current-url]'); const currentUrl = await buttonContainer.getAttribute('data-test-cta-button-current-url'); expect(currentUrl).toBe(''); // or expect appropriate error handling });
94-96: Extract URL validation regex to a shared constant.The URL validation regex is well-defined but should be:
- Extracted to a shared constant for reuse
- Enhanced to handle more edge cases (e.g., IDN domains)
+ // In a shared constants file + export const URL_VALIDATION = { + pattern: /^(https?:\/\/)([\w.-]+)(:[0-9]+)?(\/[\w.-]*)*(\?.*)?(#.*)?$/, + message: 'Invalid URL format' + }; - const validUrlRegex = /^(https?:\/\/)([\w.-]+)(:[0-9]+)?(\/[\w.-]*)*(\?.*)?(#.*)?$/; + const {pattern: validUrlRegex} = URL_VALIDATION;
99-126: Improve test organization and naming.While the test covers the requirements well, consider:
- Splitting into smaller, focused test cases
- Using a more descriptive test name
- Adding comments for each test phase
- test('button doesnt disappear when toggled, has text, has url and loses focus', async function () { + test('maintains button state and visibility after configuration', async function () {Consider splitting into:
test('maintains button visibility after losing focus', async function () { // Setup // Action: lose focus // Assert visibility }); test('preserves button configuration in non-editing mode', async function () { // Setup // Action: exit editing mode // Assert configuration });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/koenig-lexical/test/e2e/cards/button-card.test.js(1 hunks)packages/koenig-lexical/test/e2e/cards/cta-card.test.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Node 20.11.1
🔇 Additional comments (1)
packages/koenig-lexical/test/e2e/cards/button-card.test.js (1)
137-138: Good fix for test flakiness, but further improvements possible.The changes address the timing issues by using a single input event and explicit wait. However, consider these additional improvements:
- Extract test data ("Home", "Homepage") to test configuration
- Add timeout to
waitForSelectorfor better error handling- Consider testing edge cases (empty input, special characters)
+ const TEST_DATA = { + input: 'Home', + expected: 'Homepage' + }; - await page.getByTestId('button-input-url').fill('Home'); - await page.waitForSelector('[data-testid="button-input-url-listOption"]'); + await page.getByTestId('button-input-url').fill(TEST_DATA.input); + await page.waitForSelector('[data-testid="button-input-url-listOption"]', {timeout: 5000});
This was referenced Jan 24, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
ref https://linear.app/ghost/issue/PLG-313/implement-button-url-update
conditions are met, which are whether the URL field is populated, Text
is populated and the button is active.
Summary by CodeRabbit
New Features
Tests