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 Sharing #4918

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

Refactor Sharing #4918

wants to merge 3 commits into from

Conversation

Tishasoumya-02
Copy link
Contributor

@nileshgulia1 @JeffersonBledsoe , if you could look over where it is going wrong

@netlify
Copy link

netlify bot commented Jun 26, 2023

Deploy Preview for volto canceled.

Name Link
🔨 Latest commit c35b4c2
🔍 Latest deploy log https://app.netlify.com/sites/volto/deploys/649950e2a23c8e00087f6281

@cypress
Copy link

cypress bot commented Jun 26, 2023

9 failed tests on run #5921 ↗︎

9 498 20 0 Flakiness 0

Details:

Merge branch 'master' into sharing-refactor
Project: Volto Commit: c35b4c290d
Status: Failed Duration: 14:38 💡
Started: Jun 26, 2023 8:51 AM Ended: Jun 26, 2023 9:06 AM
Failed  login.js • 1 failed test • Core 18.x

View Output Video

Test Artifacts
Login Tests > As registered user I can login Output Screenshots Video
Failed  sharing.js • 1 failed test • Core 18.x

View Output Video

Test Artifacts
Sharing Tests > As editor, I can share a page to another user Output Screenshots Video
Failed  login.js • 1 failed test • Core Basic - Plone 5

View Output Video

Test Artifacts
Login Tests > As registered user I can login Output Screenshots Video
Failed  sharing.js • 1 failed test • Core Basic - Plone 5

View Output Video

Test Artifacts
Sharing Tests > As editor, I can share a page to another user Output Screenshots Video
Failed  sharing.js • 1 failed test • Seamless

View Output Video

Test Artifacts
Sharing Tests > As editor, I can share a page to another user Output Screenshots Video

The first 5 failed specs are shown, see all 9 specs in Cypress Cloud.

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

this.setState({ isClient: true });
}
useEffect(() => {
dispatch(getSharing(getBaseUrl(pathname), search));
Copy link
Member

Choose a reason for hiding this comment

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

Beware of lifecycle changes. The toast is only displayed if the request is successfully loaded. we probably don't need any other entry to dep. array other than this.

};
}),
useEffect(() => {
map(props_entries, (entry) => {
Copy link
Member

Choose a reason for hiding this comment

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

This should be moved in previous useEffect under the condition. https://github.com/plone/volto/pull/4918/files#r1241902386

*/
onSubmit(event) {
useEffect(() => {
setInherit(props_inherit === null ? inherit : props_inherit);
Copy link
Member

Choose a reason for hiding this comment

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

same as previous. Should be moved inside a condition.

const [inherit, setInherit] = useState(props_inherit);
const [isClient, setIsClient] = useState(false);
const [entries, setEntries] = useState(props_entries);
const pathname = props.location.pathname;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const pathname = props.location.pathname;
const { pathname } = useLocation()

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);
const title = useSelector((state) => state.content.data?.title, shallowEqual);
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need an extra field for title, as we are exporting whole content.

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

2 participants