-
-
Notifications
You must be signed in to change notification settings - Fork 714
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
Refactor Anontools #4845
Merged
+93
−74
Merged
Refactor Anontools #4845
Changes from all commits
Commits
Show all changes
19 commits
Select commit
Hold shift + click to select a range
0aff2e8
Refactor Anontools
Tishasoumya-02 f3f9698
Refactor anontools
Tishasoumya-02 2e56ef9
Refactor anontools
Tishasoumya-02 5511a79
Merge branch 'master' into refactor-anontools
Tishasoumya-02 4623bd6
/news
Tishasoumya-02 7140407
Merge branch 'refactor-anontools' of https://github.com/plone/volto i…
Tishasoumya-02 7115130
Merge branch 'master' of https://github.com/plone/volto into refactor…
Tishasoumya-02 8f26920
Refactor Anontools
Tishasoumya-02 15bc9ad
Refactor anontools
Tishasoumya-02 384cea5
Refactored useContent
sneridagh 815234c
Merge branch 'master' into refactor-anontools
nileshgulia1 039916c
Renamed /news bugfix->feature. Destructured useContent()
Tishasoumya-02 2cd4cd6
Prettier formated
Tishasoumya-02 3706d80
Removing destructured data
Tishasoumya-02 05b133c
Merge branch 'master' into refactor-anontools
Tishasoumya-02 d524a83
Fix tests, bring back the correct destructure
sneridagh 441dafc
Add some documentation to useContent hook
sneridagh 80f07ac
Use flattenToAppURL
sneridagh 4d46640
Merge branch 'master' into refactor-anontools
sneridagh File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
Refactored Anontools components. @Tishasoumya-02 |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,83 +1,56 @@ | ||
/** | ||
* Anontools component. | ||
* @module components/theme/Anontools/Anontools | ||
*/ | ||
|
||
import React, { Component } from 'react'; | ||
import PropTypes from 'prop-types'; | ||
import { connect } from 'react-redux'; | ||
import { Link } from 'react-router-dom'; | ||
import { Menu } from 'semantic-ui-react'; | ||
import { FormattedMessage } from 'react-intl'; | ||
import { flattenToAppURL } from '@plone/volto/helpers'; | ||
import { useToken } from '@plone/volto/hooks/userSession/useToken'; | ||
import { useContent } from '@plone/volto/hooks/content/useContent'; | ||
import config from '@plone/volto/registry'; | ||
|
||
/** | ||
* Anontools container class. | ||
*/ | ||
export class Anontools extends Component { | ||
/** | ||
* Property types. | ||
* @property {Object} propTypes Property types. | ||
* @static | ||
*/ | ||
static propTypes = { | ||
token: PropTypes.string, | ||
content: PropTypes.shape({ | ||
'@id': PropTypes.string, | ||
}), | ||
}; | ||
|
||
/** | ||
* Default properties. | ||
* @property {Object} defaultProps Default properties. | ||
* @static | ||
*/ | ||
static defaultProps = { | ||
token: null, | ||
content: { | ||
'@id': null, | ||
}, | ||
}; | ||
const Anontools = () => { | ||
const token = useToken(); | ||
const { data: content } = useContent(); | ||
|
||
/** | ||
* Render method. | ||
* @method render | ||
* @returns {string} Markup for the component. | ||
*/ | ||
render() { | ||
const { settings } = config; | ||
return ( | ||
!this.props.token && ( | ||
<Menu pointing secondary floated="right"> | ||
const { settings } = config; | ||
return ( | ||
!token && ( | ||
<Menu pointing secondary floated="right"> | ||
<Menu.Item> | ||
<Link | ||
aria-label="login" | ||
to={`/login${ | ||
content?.['@id'] | ||
? `?return_url=${flattenToAppURL(content['@id'])}` | ||
: '' | ||
}`} | ||
> | ||
<FormattedMessage id="Log in" defaultMessage="Log in" /> | ||
</Link> | ||
</Menu.Item> | ||
{settings.showSelfRegistration && ( | ||
<Menu.Item> | ||
<Link | ||
aria-label="login" | ||
to={`/login${ | ||
this.props.content?.['@id'] | ||
? `?return_url=${this.props.content['@id'].replace( | ||
settings.apiPath, | ||
'', | ||
)}` | ||
: '' | ||
}`} | ||
> | ||
<FormattedMessage id="Log in" defaultMessage="Log in" /> | ||
<Link aria-label="register" to="/register"> | ||
<FormattedMessage id="Register" defaultMessage="Register" /> | ||
</Link> | ||
</Menu.Item> | ||
{settings.showSelfRegistration && ( | ||
<Menu.Item> | ||
<Link aria-label="register" to="/register"> | ||
<FormattedMessage id="Register" defaultMessage="Register" /> | ||
</Link> | ||
</Menu.Item> | ||
)} | ||
</Menu> | ||
) | ||
); | ||
} | ||
} | ||
)} | ||
</Menu> | ||
) | ||
); | ||
}; | ||
|
||
export default Anontools; | ||
|
||
Anontools.propTypes = { | ||
token: PropTypes.string, | ||
content: PropTypes.shape({ | ||
'@id': PropTypes.string, | ||
}), | ||
}; | ||
|
||
export default connect((state) => ({ | ||
token: state.userSession.token, | ||
content: state.content.data, | ||
}))(Anontools); | ||
Anontools.defaultProps = { | ||
token: null, | ||
content: { | ||
'@id': null, | ||
}, | ||
}; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
import { useSelector, shallowEqual } from 'react-redux'; | ||
|
||
/** | ||
* useContent hook | ||
* | ||
* This hook returns the current content that is stored in the Redux store in the | ||
* `content` reducer, and returns it along with the related state (loading/loaded/error). | ||
* | ||
* @export | ||
* @return {{ data: ContentData, loading: boolean, loaded: boolean, error: Error }} | ||
*/ | ||
export function useContent() { | ||
const data = useSelector((state) => state.content.data, shallowEqual); | ||
const loading = useSelector((state) => state.content.get.loading); | ||
const loaded = useSelector((state) => state.content.get.loaded); | ||
const error = useSelector((state) => state.content.get.error, shallowEqual); | ||
|
||
return { data, loading, loaded, error }; | ||
} | ||
|
||
// For reference purposes: Potential future useQuery version | ||
// export function useContent() { | ||
// // the cache will need to know the current location | ||
// const pathname = useLocation(); | ||
// const query = useQuery(getContentQuery({ path })) | ||
|
||
// // This might not be needed if we rename the properties | ||
// const {isLoading: loading, isSuccess: loaded, ...rest} = query; | ||
|
||
// return { loading, loaded, ...rest }; | ||
// } |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
@tiberiuichim what do you think of 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.
@sneridagh I'm not sure, in principle is fine, but I have some concerns.
I'm genuinely curious about all the "ceremony", with multiple hook calls and shallowEqual. Why not just return state.content?
I think that at this point, while we wait for useQuery, the hook is underpowered. It can't be used for content subrequests, which highlights the other problem: the hook right now only takes care of yielding content info, it doesn't trigger a
getContent
action.Following this train of thought, I think it's better to either go full useQuery or not at all. If we introduce this, we'll have multiple variations of this type of code in the codebase (old style, useContent, etc), which will all have to be migrated to the new variation of
useContent
.The counter-argument is that the
useContent
with subrequests is not needed in vanilla Volto, so it's fine to introduce it right now as it is and enhance it with params and options later.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.
Thanks for the review! Let's try to talk today!