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.

Sign up for GitHub

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

Jump to bottom

enh: Migrate photos picker to use NcDialog #2296

Merged
merged 3 commits into from
Feb 7, 2024
Merged

enh: Migrate photos picker to use NcDialog #2296

merged 3 commits into from
Feb 7, 2024

Conversation

susnux
Copy link
Contributor

@susnux susnux commented Jan 31, 2024

This simplifies the code and also fixes the double scrollbar issue.

@susnux susnux added enhancement New feature or request 3. to review Waiting for reviews accessibility labels Jan 31, 2024
@susnux susnux added this to the Nextcloud 29 milestone Jan 31, 2024
@susnux susnux mentioned this pull request Jan 31, 2024
Closed
Pytal

This comment was marked as resolved.

Sign in to view

@susnux susnux requested a review from Pytal January 31, 2024 18:43
@susnux

This comment was marked as resolved.

Sign in to view
Pytal

This comment was marked as resolved.

Sign in to view

Pytal

This comment was marked as resolved.

Sign in to view

@susnux

This comment was marked as resolved.

Sign in to view
@susnux susnux requested a review from Pytal January 31, 2024 23:51
Pytal
Pytal reviewed Feb 1, 2024
View reviewed changes
src/components/PhotosPicker.vue Outdated Show resolved Hide resolved
src/components/PhotosPicker.vue Outdated Show resolved Hide resolved
src/components/PhotosPicker.vue Outdated Show resolved Hide resolved
@susnux susnux requested a review from Pytal February 3, 2024 14:26
@susnux
Copy link
Contributor Author

susnux commented Feb 3, 2024

/backport to stable28

@backportbot backportbot bot added the backport-request Pending backport by the backport-bot label Feb 3, 2024
@susnux
Copy link
Contributor Author

susnux commented Feb 3, 2024

@Pytal resolved your comments, please re-review :)

@susnux susnux force-pushed the enh/migrate-to-ncdialog branch 2 times, most recently from 11fa43c to 94ca92a Compare February 3, 2024 21:46
@szaimen
Copy link
Contributor

szaimen commented Feb 5, 2024

Sorry, I don't feel knowledgeable enough to review this...

@szaimen szaimen removed their request for review February 5, 2024 13:27
@artonge
Copy link
Collaborator

artonge commented Feb 5, 2024

Code looks good, but you probably need to update the cypress tests.

emoral435
emoral435 approved these changes Feb 5, 2024
View reviewed changes
Copy link
Contributor

@emoral435 emoral435 left a comment

Choose a reason for hiding this comment

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

PR looks fantastic! Reviewed, and tested on my local dev, everything works as intended.

NcDialog does have this weird look when the close button is focused, though 🤔
image

The active border gets clipped, let me know if you want me to fix this or just think that it is not a high priority issue right now :)

src/components/PhotosPicker.vue Outdated Show resolved Hide resolved
@susnux
Copy link
Contributor Author

susnux commented Feb 5, 2024

NcDialog does have this weird look when the close button is focused, though 🤔

This should be fixed on the nextcloud-vue-library

Pytal
Pytal requested changes Feb 5, 2024
View reviewed changes
Copy link
Member

@Pytal Pytal left a comment

Choose a reason for hiding this comment

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

Tested with a lot of photos and the photo at the end is cut off 🤔
image

@Pytal
Copy link
Member

Pytal commented Feb 6, 2024

Consider this check ✅ my approval in advance once the cut off issue is fixed :D

@susnux susnux requested a review from Pytal February 7, 2024 01:12
@Pytal
Copy link
Member

Pytal commented Feb 7, 2024

Cut off issue is still present but just tested master and it also has the same issue so will cc @artonge for investigation at a later point

Pytal
Pytal approved these changes Feb 7, 2024
View reviewed changes
Copy link
Member

@Pytal Pytal left a comment

Choose a reason for hiding this comment

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

Works well! Just have to update cypress as mentioned above

susnux and others added 3 commits February 7, 2024 11:48
@susnux
enh: Migrate photos picker to use NcDialog
5f03c45 
Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
@susnux @Pytal
fix: Add labels to NcSelect
263d7f4 
Co-authored-by: Ferdinand Thiessen <opensource@fthiessen.de>
Co-authored-by: Pytal <24800714+Pytal@users.noreply.github.com>
Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
@susnux
chore: Compile assets
02170f8 
Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
@JuliaKirschenheuter JuliaKirschenheuter merged commit 1f85e5b into master Feb 7, 2024
24 checks passed
@JuliaKirschenheuter JuliaKirschenheuter deleted the enh/migrate-to-ncdialog branch February 7, 2024 11:51
@backportbot backportbot bot mentioned this pull request Feb 7, 2024
Merged
2 tasks
@backportbot backportbot bot removed the backport-request Pending backport by the backport-bot label Feb 7, 2024
@github-actions GitHub Actions
Copy link
Contributor

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 reviewing 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 reviewing 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!

@blizzz blizzz mentioned this pull request Mar 5, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JuliaKirschenheuter JuliaKirschenheuter JuliaKirschenheuter approved these changes

@artonge artonge artonge approved these changes

@Pytal Pytal Pytal approved these changes

@emoral435 emoral435 emoral435 approved these changes

@skjnldsv skjnldsv Awaiting requested review from skjnldsv

@ShGKme ShGKme Awaiting requested review from ShGKme

Assignees
No one assigned
Labels
3. to review Waiting for reviews accessibility enhancement New feature or request feedback-requested
Projects
None yet
Milestone
Nextcloud 29
Development

Successfully merging this pull request may close these issues.

[BITV]: double scrollbar appears in a file picker dialog
6 participants
@susnux @szaimen @artonge @Pytal @JuliaKirschenheuter @emoral435