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

Refactor Anontools #4845

Merged
merged 19 commits into from
Jun 19, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions news/4845.feature
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Refactored Anontools components. @Tishasoumya-02
117 changes: 45 additions & 72 deletions src/components/theme/Anontools/Anontools.jsx
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,
},
};
18 changes: 16 additions & 2 deletions src/components/theme/Anontools/Anontools.test.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,14 @@ describe('Anontools', () => {
it('renders an anontools component when no token is specified', () => {
const store = mockStore({
userSession: { token: null },
content: { data: { '@id': 'myid' } },
content: {
data: { '@id': 'myid' },
get: {
loading: false,
loaded: true,
error: null,
},
},
intl: {
locale: 'en',
messages: {},
Expand All @@ -32,7 +39,14 @@ describe('Anontools', () => {
it('should not render an anontools component when a token is specified', () => {
const store = mockStore({
userSession: { token: '1234' },
content: { data: {} },
content: {
data: {},
get: {
loading: false,
loaded: true,
error: null,
},
},
intl: {
locale: 'en',
messages: {},
Expand Down
31 changes: 31 additions & 0 deletions src/hooks/content/useContent.js
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);
Copy link
Member

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?

Copy link
Contributor

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?

const content = useSelector((state) => state.content);
return {
  data: content.data,
  loading: content.get.loading,
  ...
}

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.

Copy link
Member

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!

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 };
// }