-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Feature/cms content #2001
Conversation
# 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
There was a problem hiding this 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
@@ -0,0 +1,13 @@ | |||
query cmsBlocks ($filter: CmsInput) { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay ;)
core/store/mutation-types.ts
Outdated
export const SYNC_PROCESS_QUEUE = SN_SYNC + '/PROCESS_QUEUE' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be in module
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
}, | ||
"cms_page": { | ||
"max_count": 500 | ||
}, |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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) | ||
} | ||
}) |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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() |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this 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
@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 |
@pkarw I hope to finish all changes today |
@yuriboyko - perfect! Thanks |
@yuriboyko can you please let us know when its ready ? |
@filrak 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
@pkarw if everything is fine from your side can you approve ? |
value: route.params.slug | ||
}).then(res => { | ||
if (res) { | ||
store.state.cmsPage.current = res |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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 () { |
There was a problem hiding this comment.
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
# Conflicts: # core/store/mutation-types.ts
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':
For CMS blocks uses elasticsearch index entity type 'cms_block':
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