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

fix(settings): Split group lists #43804

Merged
merged 3 commits into from
Mar 6, 2024
Merged

fix(settings): Split group lists #43804

merged 3 commits into from
Mar 6, 2024

Conversation

Pytal
Copy link
Member

@Pytal Pytal commented Feb 24, 2024

Summary

Before After
image image

Checklist

@Pytal Pytal added this to the Nextcloud 29 milestone Feb 24, 2024
@Pytal Pytal self-assigned this Feb 24, 2024
Copy link

Possible performance regression detected

Show Output
564 queries added

= /remote.php/dav/files/test
= /remote.php/dav/files/test/test.txt
= /remote.php/dav/files/test/many_files
= /remote.php/dav/files/test/new_file.txt
= /remote.php/dav/files/test/new_file.txt
= /remote.php/dav/files/test
= /remote.php/dav/files/test/test.txt
= /remote.php/dav/files/test/many_files
= /remote.php/dav/files/test/new_file.txt
= /remote.php/dav/files/test/new_file.txt
+ /remote.php/dav/files/test added with 45 queries
+ /remote.php/dav/files/test/test.txt added with 33 queries
+ /remote.php/dav/files/test/many_files added with 46 queries
+ /remote.php/dav/files/test/new_file.txt added with 67 queries
+ /remote.php/dav/files/test/new_file.txt added with 91 queries
+ /remote.php/dav/files/test added with 45 queries
+ /remote.php/dav/files/test/test.txt added with 33 queries
+ /remote.php/dav/files/test/many_files added with 46 queries
+ /remote.php/dav/files/test/new_file.txt added with 67 queries
+ /remote.php/dav/files/test/new_file.txt added with 91 queries

Copy link
Contributor

@JuliaKirschenheuter JuliaKirschenheuter left a comment

Choose a reason for hiding this comment

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

Great! Works ;)

@Pytal Pytal changed the base branch from master to chore/bump-ncvue-to_8.8.1 March 4, 2024 15:18
@Pytal
Copy link
Member Author

Pytal commented Mar 4, 2024

/backport to stable28

Base automatically changed from chore/bump-ncvue-to_8.8.1 to master March 4, 2024 15:30
@Pytal Pytal marked this pull request as ready for review March 4, 2024 15:46
@Pytal Pytal enabled auto-merge March 4, 2024 15:47
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.

This looks good to me 😀 is this potentially linked to one of the issues within the board?

@ShGKme ShGKme disabled auto-merge March 4, 2024 16:33
@ShGKme
Copy link
Contributor

ShGKme commented Mar 4, 2024

I don't know if it is @nextcloud/vue or local issue, but scrolling is only applyed to the NcAppNavigation itself

scrolling

It makes groups unreachable on a11y viewport size

image

@Pytal
Copy link
Member Author

Pytal commented Mar 4, 2024

Good point @ShGKme :)

Removed the scrolling on purpose because height: fit-content with overflow: auto still makes it scroll and to keep the same design but I think adding a separate container which scrolls like ul.app-navigation__list when using default slot makes sense (in this case NcAppNavigationList)

image

@JuliaKirschenheuter JuliaKirschenheuter marked this pull request as ready for review March 6, 2024 17:17
@JuliaKirschenheuter
Copy link
Contributor

I've tested it and it looks ok and fixes initial problem:

Screenshot from 2024-03-06 18-11-08

Signed-off-by: Christopher Ng <chrng8@gmail.com>
Signed-off-by: Christopher Ng <chrng8@gmail.com>
Signed-off-by: Christopher Ng <chrng8@gmail.com>
@ShGKme
Copy link
Contributor

ShGKme commented Mar 6, 2024

The bug on a small screen is not super critical, we can merge/backport the PR first and then updating on @nextcloud/vue should fix the layout for a small screen.

@skjnldsv skjnldsv disabled auto-merge March 6, 2024 19:51
@skjnldsv skjnldsv merged commit 8d0746f into master Mar 6, 2024
90 of 96 checks passed
@skjnldsv skjnldsv deleted the fix/a11y/groups-heading branch March 6, 2024 19:51
@skjnldsv
Copy link
Member

skjnldsv commented Mar 7, 2024

Broke cypress 😁

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.

None yet

5 participants