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

Feature/cms content #2001

Merged
merged 23 commits into from
Nov 29, 2018
Merged

Conversation

yuriboyko
Copy link
Contributor

@yuriboyko yuriboyko commented Nov 16, 2018

Related issues

#1987
#1894

Short description and why it's useful

This PR adds functionality for using CMS content from Elasticsearch with supporting SSR
It uses dynamic routes for CMS pages

For CMS block usage example please check demo page Vue file
src/themes/default/pages/CmsBlockDemoPageSsr.vue

Data structure

For CMS pages uses elasticsearch index entity type 'cms_page':

`id`
`title`
`meta_keywords`
`meta_description`
`identifier`
`content_heading`
`content`
`creation_time`
`update_time`
`active`

For CMS blocks uses elasticsearch index entity type 'cms_block':

`id`
`title`
`identifier`
`content`
`creation_time`
`update_time`

Announcement

Later will be provided functionality for page hierarchy

Magento support

Data migration tools for Magento 1 and Magento 2 will be updated accordingly very soon with correspond PRs

# Conflicts:
#	core/store/lib/search/adapter/graphql/processor/processType.ts
#	core/store/lib/search/adapter/graphql/searchAdapter.ts
#	core/store/modules/index.ts
# Conflicts:
#	src/themes/default/router/index.js
…s , fix bugs

# Conflicts:
#	core/store/mutation-types.ts
#	src/themes/default/router/index.js
…ssues

# Conflicts:
#	core/store/mutation-types.ts
#	src/themes/default/router/index.js
Copy link
Collaborator

@filrak filrak left a comment

Choose a reason for hiding this comment

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

Awesome to finally have this, thank you! I added my comments mostly regarding encapsulating cms logic inside module instead of splitting it across whole app. Please adjust the PR

core/components/blocks/Cms/Block.js Outdated Show resolved Hide resolved
core/modules/cms/store/block/actions.ts Outdated Show resolved Hide resolved
core/modules/cms/store/block/mutations.ts Outdated Show resolved Hide resolved
core/pages/CmsPage.js Outdated Show resolved Hide resolved
core/pages/CmsPage.js Outdated Show resolved Hide resolved
core/store/index.ts Outdated Show resolved Hide resolved
@@ -0,0 +1,13 @@
query cmsBlocks ($filter: CmsInput) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

should be in module/queries

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This gql is related to the ceratin searchAdapter. Not sure if it should be in the module/queries

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree with @filrak; if there is currently no way to fully add query/resultAdapter from the module level - then we need to extend the graphQL API to have this capability. Anyway - everything should be registered from the module level not modifying the core; CMS module should be a example how decoupled the app is with new module system

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok i see the idea. then probably it makes sense
i will try move register entity Type to the module level

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Lets move this update to another PR as it requre upadt not only cms module it should be applied for all other modules uses queries as well

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add an issue for this

@@ -0,0 +1,14 @@
query cmsHierarchies ($filter: CmsInput) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

should be in module/queries

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This gql is related to the certain searchAdapter. Not sure if it should be in the module/queries

Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as above - it should be within the module only. We need to have a capability of extending graphQL entities for custom ones from the module level

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same as above - Lets move this update to another PR as it requre upadt not only cms module it should be applied for all other modules uses queries as well

@@ -0,0 +1,14 @@
query cmsPages ($filter: CmsInput) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

should be in module/queries

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This gql is related to the certain searchAdapter. Not sure if it should be in the module/queries

Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same as above - Lets move this update to another PR as it requre upadt not only cms module it should be applied for all other modules uses queries as well

Copy link
Collaborator

Choose a reason for hiding this comment

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

okay ;)

export const SYNC_PROCESS_QUEUE = SN_SYNC + '/PROCESS_QUEUE'
Copy link
Collaborator

Choose a reason for hiding this comment

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

should be in module

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is not our code part. also as i see SYNC_PROCESS_QUEUE mutation type is not used at all. maybe you can simply remove it

Copy link
Collaborator

Choose a reason for hiding this comment

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

i meant all the rest, sorry for misleading. currently only /sync tasks should be left in vuex module

@filrak filrak changed the title Feature/cms content WIP: Feature/cms content Nov 18, 2018
},
"cms_page": {
"max_count": 500
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

We shouldn’t load all cms blocks and pages as it will cause sending them with the INITIAL_STATE output and can be harmful boot h for performance + SEO

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this config option will need to easily manage load of all blocks list if it needs as we need to have size provided for ES to get the data overwise it will be only 10

resultPorcessor: (resp, start, size) => {
return this.handleResult(resp, 'cms_hierarchy', start, size)
}
})
Copy link
Collaborator

Choose a reason for hiding this comment

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

These entities should be registered from the module level - keeping all the CMS oriented logic inside the cms module - not in the core I believe (currently CMS is also a module without references from the core)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same as for gql comments - Lets move this update to another PR

Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you create an issue for that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Issue is created

}
},
asyncData ({ store, route, context }) {
// @TODO to cover SSR need find a way to pass props identifier/id to the asyncData()
Copy link
Collaborator

Choose a reason for hiding this comment

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

asyncData() won’t be called for child components; so if You place 3 different CmsBlocks on one single page - asyncData won’t be called for these CmsBlocks. Therefore we need this dataLoader pattern implemented - and then we can call store.dispatch(‘dm/registerPromise’, this.loadData()) in created() event handler as we discussed with @yuriboyko on Slack

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As i see asyncData() is called for child components in our case but the main problem is what we are not able to use component props to fetch correspond block data by identifier. Suggested solution by @pkarw is stll in the progress. For now I suggest this using SSR for CMS blocks it as a stuff for next pull request

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As i mentioned. For now CMS blocks recoded to not use SSR as solution initially suggested does not work and the way with loading all blocks also is not good. Need find a new solution and it will be a new PR.

Copy link
Collaborator

@pkarw pkarw left a comment

Choose a reason for hiding this comment

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

We need to move the graphQL objects registration to the module level, add on-demand data loading (via dataLoader - data loading promises to be registered inside created() event in components), we also need to load the blocks and pages on demand - not as they’re loaded all at once currently

@pkarw
Copy link
Collaborator

pkarw commented Nov 20, 2018

@yuriboyko how do You see the changes required in here? When can we expect them to happen? :) I'm asking because lot of users are waiting for it - and we're releaseing 1.6 on 6th of December :) So the next week is great chance to make it happen to have another week for tests :D

@yuriboyko
Copy link
Contributor Author

@pkarw I hope to finish all changes today

@pkarw
Copy link
Collaborator

pkarw commented Nov 21, 2018

@yuriboyko - perfect! Thanks

@filrak
Copy link
Collaborator

filrak commented Nov 26, 2018

@yuriboyko can you please let us know when its ready ?

@yuriboyko
Copy link
Contributor Author

@filrak
At this moment all possible changes are done and commited.
As i mentioned before regarding moving queries to module level it will be separate PR
Regarding using SSR for blocks and do not using preload i already talked to Piotr
and i think for now would be better to not use SSR for blocks, as to use it we need to preload all blocks to solve issues with not accessible props, but unfortunately it is also not possible to read cachestorage on the asyncData level. This means even if we would use SSR with load all blocks and saving them to cachestorage next time with page load there will be not way to verify if blocks are loaded to cachestorage (as cachestoarge not acessible) so blocks loads will happen again which is not good. So for now i suggest to not change anything and then if we find some other solution for blocks SSR it will be another PR.

So for now i think current state is ready for approve PR to have this CMS functionality. if you see some other issues let me know

# Conflicts:
#	src/themes/default/router/index.js
# Conflicts:
#	src/themes/default/router/index.js
@filrak
Copy link
Collaborator

filrak commented Nov 28, 2018

@pkarw if everything is fine from your side can you approve ?

@filrak filrak changed the title WIP: Feature/cms content Feature/cms content Nov 28, 2018
value: route.params.slug
}).then(res => {
if (res) {
store.state.cmsPage.current = res
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be moved to mutation called from the cmsPage/single action, not in here @yuriboyko

},
methods: {
savePage (page) {
return this.$store.state['cmsPage'] && Object.keys(page).length > 0 ? this.$store.dispatch('cmsPage/addItem', page) : false
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should be moved to vuex

@@ -0,0 +1,13 @@
query cmsBlocks ($filter: CmsInput) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add an issue for this

})
}) */
// },
created () {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add an issue for SSR support for CMS blocks @yuriboyko please

@pkarw pkarw merged commit 299182e into vuestorefront:develop Nov 29, 2018
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.

None yet

4 participants