-
-
Notifications
You must be signed in to change notification settings - Fork 718
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
Use navroot to render the sitemap (targets 17.x.x
)
#5185
Changes from 25 commits
ee2675a
5265143
d8ca93f
ffa4463
c1a2552
8592c34
3f03d9f
fc4992d
d3ff128
58cfb4c
de072db
435699b
2c33991
2db6c3a
9e95e9c
bb9e6af
be53691
cba0763
4d4110f
8e2a13e
33c1306
9e7d3e7
86d4a9e
025ba57
3833b31
da608fc
568aee0
7eed6c2
b354ed4
41ce501
8850c07
007b388
334057a
9a51a6d
b7ae8c0
673ac6c
a059b03
d4e5c02
68f57b1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
Use navroot to render the sitemap @erral |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,14 +7,14 @@ import React, { Component } from 'react'; | |
import PropTypes from 'prop-types'; | ||
import { compose } from 'redux'; | ||
import { connect } from 'react-redux'; | ||
import { asyncConnect } from '@plone/volto/helpers'; | ||
import { asyncConnect, flattenToAppURL } from '@plone/volto/helpers'; | ||
import { defineMessages, injectIntl } from 'react-intl'; | ||
import { Container } from 'semantic-ui-react'; | ||
import { Helmet, toBackendLang } from '@plone/volto/helpers'; | ||
import { Helmet } from '@plone/volto/helpers'; | ||
import { Link } from 'react-router-dom'; | ||
import config from '@plone/volto/registry'; | ||
|
||
import { getNavigation } from '@plone/volto/actions'; | ||
import { getNavigation, getNavroot } from '@plone/volto/actions'; | ||
|
||
const messages = defineMessages({ | ||
Sitemap: { | ||
|
@@ -23,12 +23,6 @@ const messages = defineMessages({ | |
}, | ||
}); | ||
|
||
export function getSitemapPath(pathname = '', lang) { | ||
const prefix = pathname.replace(/\/sitemap$/gm, '').replace(/^\//, ''); | ||
const path = prefix || lang || ''; | ||
return path; | ||
} | ||
|
||
/** | ||
* Sitemap class. | ||
* @class Sitemap | ||
|
@@ -45,14 +39,10 @@ class Sitemap extends Component { | |
}; | ||
|
||
componentDidMount() { | ||
const { settings } = config; | ||
|
||
const lang = settings.isMultilingual | ||
? `${toBackendLang(this.props.lang)}` | ||
: null; | ||
|
||
const path = getSitemapPath(this.props.location.pathname, lang); | ||
this.props.getNavigation(path, 4); | ||
this.props.getNavigation( | ||
flattenToAppURL(this.props.navroot?.navroot?.['@id']), | ||
config.settings.siteMapDepth, | ||
); | ||
} | ||
|
||
/** | ||
|
@@ -95,9 +85,9 @@ export const __test__ = compose( | |
connect( | ||
(state) => ({ | ||
items: state.navigation.items, | ||
lang: state.intl.locale, | ||
navroot: state.navroot.data, | ||
}), | ||
{ getNavigation }, | ||
{ getNavigation, getNavroot }, | ||
), | ||
)(Sitemap); | ||
|
||
|
@@ -106,25 +96,20 @@ export default compose( | |
connect( | ||
(state) => ({ | ||
items: state.navigation.items, | ||
lang: state.intl.locale, | ||
navroot: state.navroot.data, | ||
}), | ||
{ getNavigation }, | ||
{ getNavigation, getNavroot }, | ||
), | ||
asyncConnect([ | ||
{ | ||
key: 'navigation', | ||
promise: ({ location, store: { dispatch, getState } }) => { | ||
if (!__SERVER__) return; | ||
const { settings } = config; | ||
|
||
const path = getSitemapPath( | ||
location.pathname, | ||
settings.isMultilingual | ||
? toBackendLang(getState().intl.locale) | ||
: null, | ||
const navroot = getState().navroot.data?.navroot?.navroot?.['@id']; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is the navroot available in both 7.x and 8.x branches of plone.restapi? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No right now it is not. But I can try to add a PR to backport them . There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why does it need to be backported? This fix will go in Volto 17 and maybe Volto 16, which both expect plone.restapi 8.x, even on Plone 5. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. https://github.com/plone/plone.restapi/blob/ed192208499989b379de8534a3aa12d037316db4/setup.py#L11 Volto 16 works fine with Plone 4, we're using it in production on a complex site. If this PR is backported to Volto 16, it will be a breaking change. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay, I didn't know you are using Volto 16 with Plone 4. That is not officially supported, as far as I am aware, and there are no automated tests for it in volto. So this is a risk you took on in using Volto 16 with an unsupported Plone version. But, I agree we should avoid breaking that, unless it's really hard to avoid. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The navroot endpoints are not that complex, I would say that they could be backported to 7.x easily. But let's see what the tests say 😅 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
return dispatch( | ||
getNavigation(flattenToAppURL(navroot), config.settings.siteMapDepth), | ||
); | ||
|
||
return dispatch(getNavigation(path, 4)); | ||
}, | ||
}, | ||
]), | ||
|
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 would be a breaking change because the
getSitemapPath
is removed. Let's keep it and mark it as deprecated? @sneridagh your thoughts on this?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 have kept the function and the tests, but mark it as deprecated with a docstring. This way, as you say, if anyone is using the function, it keeps working.
Would that be enough?