Skip to content

Conversation

mohamedsalem401
Copy link
Member

No description provided.

);

dataListeners.push(
this.createListener(em, 'data:path', ({ path: eventPath }: { path: string }) => {
Copy link
Member

Choose a reason for hiding this comment

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

let's use event enums DataSourcesEvents.path

export interface DataCollectionStateMap {
export type RootDataType = Array<ObjectAny> | ObjectAny;

export type DataCollectionStateMap = {
Copy link
Member

Choose a reason for hiding this comment

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

any reason to change this interface to a type?


syncComponentsCollectionState() {
super.syncComponentsCollectionState();
this.components().forEach((cmp) => cmp.syncComponentsCollectionState?.());
Copy link
Member

Choose a reason for hiding this comment

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

It seems like this.components().forEach is executed twice (super/Component + this one)

super(props, opt);

const hasDataResolver = this.getDataResolver();
this.syncComponentsCollectionState();
Copy link
Member

Choose a reason for hiding this comment

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

Don't we need to add this inside the condition below?

import EditorModel from '../../editor/model/Editor';
import { CssRuleJSON } from '../../css_composer/model/CssRule';
import { ComponentDefinition } from '../../dom_components/model/types';
import { DataCollectionStateMap } from '../../data_sources/model/data_collection/types';
Copy link
Member

Choose a reason for hiding this comment

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

is it unused?

Comment on lines +179 to +181
expect(secondChild().get('name')).toBe('user2');
expect(secondChild().get('custom_property')).toBe('user2');
expect(secondGrandchild().get('name')).toBe('user2');
Copy link
Member

Choose a reason for hiding this comment

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

If you had to make these changes, that means it started to recreate all inner components on data source change

Comment on lines +94 to +102
test('sets dataResolver and updates wrapper.page/head collectionsStateMap', () => {
wrapper.setDataResolver(createDataResolver('pagesDataSource.pages.data'));
wrapper.resolverCurrentItem = 0;
const stateMap = wrapper.collectionsStateMap;

expect(stateMap).toHaveProperty(keyRootData);
expect(wrapper.head.collectionsStateMap).toEqual(stateMap);
expect(wrapper.head.collectionsStateMap).toEqual({ [keyRootData]: firstPageData });
});
Copy link
Member

Choose a reason for hiding this comment

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

I'd avoid "testing" collectionsStateMap, it's more of an implementation detail (eg, if I change its name, I'd not expect to change our tests)

Comment on lines +50 to +70
const firstPageData = { id: 'page1', title: 'Title1' };
const pagesData = [firstPageData, { id: 'page2', title: 'Title2' }, { id: 'page3', title: 'Title3' }];
const objectData = {
page1: { title: 'page1' },
page2: { title: 'page2' },
};

beforeEach(() => {
({ em, dsm } = setupTestEditor());
wrapper = em.getWrapper() as ComponentWrapper;

pagesDataSource = dsm.add({
id: 'pagesDataSource',
records: [
{ id: 'pages', data: pagesData },
{
id: 'objectData',
data: objectData,
},
],
});
Copy link
Member

Choose a reason for hiding this comment

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

The data set here is a bit confusing. Can you differentiate it a bit (blogs, products, etc.)?

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