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

Update to @nextcloud/dialogs v5.3.5 and @nextcloud/vue v8.15.0 and one page usage init #47

Merged
merged 1 commit into from
Oct 7, 2024

Conversation

Thatoo
Copy link
Contributor

@Thatoo Thatoo commented Aug 20, 2024

Thanks to @nextcloud/dialogs v5.3.5 and @nextcloud/vue v8.15.0 it is now possible to build picker window with several button having each one its own action.
This is why i propose to make the choice between read and write directly on the front page to gain one step.

screenshot-2024-08-20-17-50-49

I also merged (and adapt) my work from #38 to give the option to have either one window that open files (no url parameter) or one window (adding an url parameter : ?option=Clipboard) that copy one of the three link (read, write or internal) into the Clipboard.

screenshot-2024-08-20-13-02-10

To answer the need of other project who would like to use this picker app, I invite to offer the option to close automatically this "Clipboard" window (or iframe) at the same time the user copies the link to the clipboard. I explain in the README how dev can implement this option for the window the'd open or iframe they'd call (with the url parameter : ?option=Clipboard) to be closed automatically.

Before merging, these 4 sentences should be translated :

t('picker', 'Choose a file you want to copy a link from')
t('picker', 'Copy Read Only public link'),
t('picker', 'Copy Editable public link'),
t('picker', 'Copy Internal link'),

@Thatoo Thatoo marked this pull request as ready for review August 20, 2024 17:59
Copy link

github-actions bot commented Sep 4, 2024

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

(If you believe you should not receive this message, you can add yourself to the blocklist.)

@DaphneMuller
Copy link

hi, I'm very sorry for the slow review on your PR. I've pinged the team lead who is responsible for this repository and asked them to get back to you

@provokateurin
Copy link
Member

Sorry, I was planning to look into this, but didn't find the time yet.
@Thatoo also created multiple similar PRs and pushed a bunch of times, so it didn't look to me like the changes were completed.

@provokateurin
Copy link
Member

@Thatoo am I right that this PR replaces #38 and #45? If that is the case please close them so we can focus on this one.

Copy link
Member

@provokateurin provokateurin left a comment

Choose a reason for hiding this comment

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

I haven' tried it yet, but here is some initial feedback.

Copy link
Member

@provokateurin provokateurin left a comment

Choose a reason for hiding this comment

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

@Thatoo thank you so much, this is really awesome work!
I just have a few comments that need to be addressed:

  1. The package-lock.json is not up-to-date, please run npm i and commit the changes.
  2. Eslint shows a few warnings to me locally, please fix them (CI will also complain when it runs)

@Thatoo
Copy link
Contributor Author

Thatoo commented Sep 29, 2024

I corrected most of what you mentioned. Eslint warning remain to be fixed.

@Thatoo
Copy link
Contributor Author

Thatoo commented Sep 29, 2024

I tried again with Nextcloud 28 and I don't have any Eslint warning to fix :

added 1191 packages, and audited 1192 packages in 2m

298 packages are looking for funding
  run `npm fund` for details

21 vulnerabilities (1 low, 15 moderate, 5 high)

To address issues that do not require attention, run:
  npm audit fix

To address all issues possible (including breaking changes), run:
  npm audit fix --force

Some issues need review, and may require choosing
a different dependency.

Run `npm audit` for details.
/usr/bin/npm run dev

> picker@0.0.1 dev
> NODE_ENV=development webpack --progress --config webpack.js

Building picker 0.0.1 

assets by info 7.74 MiB [immutable]
  asset picker-vendors-node_modules_nextcloud_dialogs_dist_chunks_FilePicker-DUbP4INd_mjs.js?v=9e492d54ead90629c3b7 7.73 MiB [emitted] [immutable] (id hint: vendors) 1 related asset
  asset picker-data_image_svg_xml_3c_21--_20-_20SPDX-FileCopyrightText_202020_20Google_20Inc_20-_20SPDX-Lice-cc29b1.js?v=06baefd4ba873c00ce66 7.53 KiB [emitted] [immutable]
  asset picker-node_modules_nextcloud_dialogs_dist_chunks_index-CYiQsZoY_mjs.js?v=77af785dc58c6db13ec2 1.5 KiB [emitted] [immutable] 1 related asset
asset picker-limit.js 6.29 MiB [emitted] (name: limit) 1 related asset
asset picker-main.js 3.01 MiB [emitted] (name: main) 1 related asset
asset picker-webexShare.js 410 KiB [emitted] (name: webexShare) 1 related asset
asset picker-pickerShare.js 119 KiB [emitted] (name: pickerShare) 1 related asset
webpack 5.93.0 compiled successfully in 74074 ms
make[1]: Leaving directory '/var/www/nextcloud/apps/picker'

Could you tell me what you have? WHat version of Nextcloud are you using?

@provokateurin
Copy link
Member

Nextcloud version doesn't matter for that. I just ran npm run lint and got a few errors. I'll let the CI run to demonstrate the problem.

@Thatoo
Copy link
Contributor Author

Thatoo commented Sep 29, 2024

Indeed

$ npm run lint

> picker@0.0.1 lint
> eslint --ext .js,.vue src

/var/www/nextcloud/apps/picker/src/WebexShare.vue
  28:29  error  "vue-material-design-icons/LinkVariant" is not found                      n/no-missing-import
  29:27  error  "vue-material-design-icons/OpenInNew" is not found                        n/no-missing-import
  30:20  error  Unable to resolve path to module '@nextcloud/vue/dist/Components/Button'  import/no-unresolved
  30:20  error  "@nextcloud/vue/dist/Components/Button" is not found                      n/no-missing-import
  38:3   error  Name "Button" is reserved in HTML                                         vue/no-reserved-component-names

✖ 5 problems (5 errors, 0 warnings)

The thing is that I didn't work on WebexShare.vue and as I didn't get them when building it, I didn't know about these error.
Is this WebexShare.vue file still even needed?

@Thatoo
Copy link
Contributor Author

Thatoo commented Sep 29, 2024

I solved this lint warning even though I don't know if WebexShare.vue file is still even needed.

@provokateurin
Copy link
Member

Right, maybe these issue don't even originate from your changes. Just saw that this repo apparently doesn't have CI for eslint 🤦‍♀️

The changes look good to go, can you squash them all into a single commit and ensure the DCO is set on it? Then I can merge this and make a new release 🚀

@Thatoo
Copy link
Contributor Author

Thatoo commented Sep 30, 2024

I think it's ok now.

@Thatoo
Copy link
Contributor Author

Thatoo commented Oct 7, 2024

Do I need to do something else for you to merge it and make a new release?

@provokateurin
Copy link
Member

No, sorry I will have another look this week and when I merge it you can expect a new release on the same day.

…e page usage init

Signed-off-by: Thatoo <thatoo@leprette.fr>
@provokateurin provokateurin merged commit d5169a4 into nextcloud:main Oct 7, 2024
13 checks passed
@provokateurin provokateurin mentioned this pull request Oct 7, 2024
@provokateurin
Copy link
Member

Released as v1.0.11

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants