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

Public view fixes #2021

Merged
merged 6 commits into from May 26, 2020
Merged

Public view fixes #2021

merged 6 commits into from May 26, 2020

Conversation

y-lohse
Copy link
Contributor

@y-lohse y-lohse commented May 22, 2020

Rebasing #2017 was not straightforward and it appears I did loose a few changes in the process.

See individual commit messages for details.

mapStateToProps,
mapDispatchToProps
)(LightFileList)
)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand this change. It seems we don't use router in this component and we don't pass it neither to its children. Can you explain why this is needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is used here, in the mapDispatchToProps of he component. But I agree, it's not very obvious. Do you think it would be better to include the Actions file in this one to make it more explicit? Otherwise we can add a comment imo.

Copy link
Contributor

Choose a reason for hiding this comment

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

The best solution is to not rely on ownprops.something (since it can be dangerous for the perf) but I don't know how to handle that.

src/drive/web/modules/public/LightFileList.jsx Outdated Show resolved Hide resolved
@y-lohse y-lohse merged commit 9d626fb into master May 26, 2020
@delete-merged-branch delete-merged-branch bot deleted the fix-public branch May 26, 2020 08:52
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