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

[Vue] RouteLinkedRichText #1037

Conversation

sc-nikolaoslazaridis
Copy link
Contributor

Description / Motivation

  • Changed how RichText handles internal links.
  • Added unit tests for new functionality.

Testing Details

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Copy link
Contributor

@illiakovalenko illiakovalenko 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, see some comments

@@ -111,4 +112,85 @@ describe('<RichText />', () => {
expect(rendered.attributes()).toMatchObject(attrs);
expect(rendered.element.innerHTML).toBe(props.field.value);
});

it('should initialize links', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

This test doesn't check that link is initialized, you just check markup
in order to check that link is initialized, you should try to click, you need to understand that this.routeHandler is assigned to the event listener, you have such test at the bottom

expect(rendered.element.innerHTML).toContain('<a href="/t10">2</a>');
});

it('should not initialize links when does not have value', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

This test not related to the links, it's just a test which check that nothing is rendered
you just need to rename it properly

const hasChildNodes = rendered.element.hasChildNodes();
expect(hasChildNodes).toBe(false);
});
it('should not initialize links if no links in markup', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

This test is not related to the links, we have the same case covered in another test, can be removed
and if you check that you don't have <a href='' /> - it doesn't check that links are not initialized

expect(rendered.element.innerHTML).toContain('<h1>Hello!</h1>');
expect(rendered.element.innerHTML).not.toContain('<a href=');
});
it('should navigate if anchor element clicked', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

This test can be renamed to -> Should initialize links

Copy link
Contributor

Choose a reason for hiding this comment

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

And inside this test you can check both cases, when you have nested element and link, so you can click on nested, after that on parent and check it here

expect(mockRouter.push).toHaveBeenCalledTimes(1);
expect(mockRouter.push).toHaveBeenCalledWith('/styleguide');
});
it('should navigate if nested element clicked', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

See comment above

Copy link
Contributor

@illiakovalenko illiakovalenko left a comment

Choose a reason for hiding this comment

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

👍

@illiakovalenko illiakovalenko changed the title Handle Vue RichText links [Vue] - RouteLinkedRichText May 27, 2022
@illiakovalenko illiakovalenko changed the title [Vue] - RouteLinkedRichText [Vue] RouteLinkedRichText May 27, 2022
@illiakovalenko illiakovalenko merged commit 20705cc into Sitecore:dev May 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants