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

Read-write actions on public view #2017

Merged
merged 30 commits into from
May 19, 2020
Merged

Read-write actions on public view #2017

merged 30 commits into from
May 19, 2020

Conversation

y-lohse
Copy link
Contributor

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

With this PR, the public sharing view supports actions that require write permissions (such as renaming files, uploading, etc).

In order to re-use the menu items that we have in the protected pages, I migrated them all to use ActionMenu from cozy-ui. This is also why the PR is fairly big, apologies for that.

@y-lohse y-lohse requested review from Crash-- and ptbrowne as code owners May 12, 2020 07:21
@@ -0,0 +1,63 @@
import React from 'react'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Most of this file is lifted from https://github.com/cozy/cozy-drive/blob/master/src/drive/web/modules/drive/Container.jsx — I didn't spend too much time on it because it's been rewritten in #1979

)
if (fetchStatus === 'loaded') {
notesAppUrl = url
} else if (fetchStatus === 'errored') {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know if the commit message is explicit enough, but on the public page we don't have permissions to query the list of apps, and this is one of the only places where we really need it. I think the fallback to a generated link is ok, the only issue is if the notes app is not installed, then we won't have a link to the store.

Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if click on the button with no network?

Can't we do a check if we're in a public context or not?

(and no the commit was not self explaining, maybe you can add your comment to it ;))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What happens if click on the button with no network?

It will try to open notes :/.

Can't we do a check if we're in a public context or not?

Yep, we can! The offline behavior would be the same though. But it's probably a better solution.

(and no the commit was not self explaining, maybe you can add your comment to it ;))

👍

Copy link
Contributor

Choose a reason for hiding this comment

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

cc @poupotte pour info ;)

@y-lohse y-lohse self-assigned this May 12, 2020
Copy link
Contributor

@ptbrowne ptbrowne left a comment

Choose a reason for hiding this comment

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

I browsed the PR to have a better idea of the code in Drive that I don't know so much. Great job on using ActionMenu and deleting old code !

I saw that withBreakpoints and translate were used a lot, maybe the codemods to use useI18n and useBreakpoint could be useful in the future.

src/drive/targets/public/index.jsx Outdated Show resolved Hide resolved
@ptbrowne
Copy link
Contributor

Ah I forgot, when on a PR I now always check to see if some spec files have been changed 😈 😇 , and for such a big change, I am a bit surprised to see that none have been changed. Is there a particular reason for this ?

I find it also difficult to write test files, particularly for UI stuff. I am more and more convinced that Argos would be good to have on our apps.

@y-lohse y-lohse force-pushed the public-write branch 2 times, most recently from a3b2111 to 4fcd82f Compare May 13, 2020 20:11
@y-lohse y-lohse force-pushed the public-write branch 2 times, most recently from 6553276 to b36c30b Compare May 14, 2020 07:26
.travis.yml Show resolved Hide resolved
@y-lohse
Copy link
Contributor Author

y-lohse commented May 14, 2020

Ah I forgot, when on a PR I now always check to see if some spec files have been changed 😈 😇 ,
and for such a big change, I am a bit surprised to see that none have been changed. Is there a particular reason for this ?

I find it also difficult to write test files, particularly for UI stuff. I am more and more convinced that Argos would be good to have on our apps.

Yeah the changes were mostly UI related, to screenshots would make more sense here. But I'll add some :)

src/drive/web/modules/public/LightFolderView.jsx Outdated Show resolved Hide resolved
src/drive/web/modules/public/LightFolderView.jsx Outdated Show resolved Hide resolved
src/drive/web/modules/public/PublicToolbar.jsx Outdated Show resolved Hide resolved
)
if (fetchStatus === 'loaded') {
notesAppUrl = url
} else if (fetchStatus === 'errored') {
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if click on the button with no network?

Can't we do a check if we're in a public context or not?

(and no the commit was not self explaining, maybe you can add your comment to it ;))

package.json Show resolved Hide resolved
.travis.yml Show resolved Hide resolved
@y-lohse y-lohse requested a review from Crash-- May 18, 2020 07:15
files,
dirId,
sharingState,
fileUploadedCallback = () => null
Copy link
Contributor

Choose a reason for hiding this comment

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

comments to add

@y-lohse y-lohse merged commit df75f89 into master May 19, 2020
@delete-merged-branch delete-merged-branch bot deleted the public-write branch May 19, 2020 06:45
@y-lohse y-lohse mentioned this pull request May 22, 2020
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.

3 participants