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.

Sign up for GitHub

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

Added URL input handling on CTA Card #1417

Merged
merged 3 commits into from
Jan 23, 2025
Merged

Added URL input handling on CTA Card #1417

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
aa1ca14
Fixed conflict
ronaldlangeveld Jan 23, 2025
193d7cd
Added URL input handling on CTA Card
ronaldlangeveld Jan 23, 2025
86a98df
Improved suggested urls test
ronaldlangeveld Jan 23, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/koenig-lexical/src/components/ui/cards/CtaCard.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -269,7 +269,7 @@ export function CtaCard({

{/* Button */}
{ (showButton && (isEditing || (buttonText && buttonUrl))) &&
<div>
<div data-test-cta-button-current-url={buttonUrl}>
<Button
color={'accent'}
dataTestId="cta-button"
Expand Down
9 changes: 8 additions & 1 deletion packages/koenig-lexical/src/nodes/CallToActionNodeComponent.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,13 @@ export const CallToActionNodeComponent = ({
});
};

const handleButtonUrlChange = (val) => {
editor.update(() => {
const node = $getNodeByKey(nodeKey);
node.buttonUrl = val;
});
};

return (
<>
<CtaCard
Expand All @@ -68,7 +75,7 @@ export const CallToActionNodeComponent = ({
showButton={showButton}
text={textValue}
updateButtonText={handleButtonTextChange}
updateButtonUrl={() => {}}
updateButtonUrl={handleButtonUrlChange}
updateHasSponsorLabel={() => {}}
updateLayout={() => {}}
updateShowButton={toggleShowButton}
Expand Down
64 changes: 64 additions & 0 deletions packages/koenig-lexical/test/e2e/cards/cta-card.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,4 +63,68 @@ test.describe('Call To Action Card', async () => {
await page.fill('[data-testid="button-text"]', 'Click me');
expect(await page.textContent('[data-testid="cta-button"]')).toBe('Click me');
});

test('can set button url', async function () {
await focusEditor(page);
await insertCard(page, {cardName: 'call-to-action'});
await page.click('[data-testid="button-settings"]');
await page.fill('[data-testid="button-url"]', 'https://example.com/somepost');
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');
});

// 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);
});
Copy link

@coderabbitai coderabbitai bot Jan 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Address potential flakiness in URL suggestions test.

A few concerns with the current implementation:

  1. The comment about timing suggests potential test flakiness
  2. Test relies on hardcoded test data ("Ho", "me", "Homepage")
  3. URL validation regex might be too permissive

Consider these improvements:

  1. Use waitFor or similar mechanism instead of relying on multiple input events
  2. Inject test data through test configuration
  3. 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);
});

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed it, thanks.

Copy link

Choose a reason for hiding this comment

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

@ronaldlangeveld You're welcome! The test looks much more robust now. 🎉


test('button doesnt disappear when toggled, has text, has url and loses focus', async function () {
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');
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');

// lose focus and editing mode
await page.keyboard.press('Escape');
await page.keyboard.press('Enter');

// check if editing is false
await assertHTML(page, html`
<div data-lexical-decorator="true" contenteditable="false">
<div data-kg-card-editing="false" data-kg-card-selected="false" data-kg-card="call-to-action">
</div>
</div>
<p><br /></p>
<p><br /></p>
`, {ignoreCardContents: true});

expect(await page.isVisible('[data-testid="cta-button"]')).toBe(true);
});
});
Loading