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 table view #37870

Merged
merged 1 commit into from
May 12, 2023
Merged

Conversation

JuliaKirschenheuter
Copy link
Contributor

@JuliaKirschenheuter JuliaKirschenheuter commented Apr 21, 2023

Summary

Create a grid for users view.

One visual change: made input fields visible in editing mode.

Before After
Peek 2023-04-26 10-34 Peek 2023-04-26 10-32

AFTER STRUCTURE CHANGES (TABLE) STYLES RESPONSIBLE FOR ACTIONS WHILE EDITING A ROW WERE BROKEN
Related (same issue here): #37860

I've adapted styles and now action menu is not broken anymore. But i had to remove position: sticky for it.

Before After
Peek 2023-05-10 14-54 Peek 2023-05-10 14-56

It is absolutely valid to have a vertical scrolling inside of a table. position:sticky in this case causes too much problems and because of it i would like to leave implementation like this.

Checklist

@JuliaKirschenheuter JuliaKirschenheuter self-assigned this Apr 21, 2023
@JuliaKirschenheuter JuliaKirschenheuter added the 2. developing Work in progress label Apr 21, 2023
@JuliaKirschenheuter JuliaKirschenheuter marked this pull request as ready for review April 21, 2023 14:55
Copy link
Contributor

@artonge artonge left a comment

Choose a reason for hiding this comment

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

Could the input fields be a little longer to fill up empty spaces?

Copy link
Contributor

@artonge artonge left a comment

Choose a reason for hiding this comment

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

Looks good anyway 👍

@AndyScherzinger AndyScherzinger added this to the Nextcloud 27 milestone Apr 24, 2023
@Pytal
Copy link
Member

Pytal commented Apr 26, 2023

Is the developing label still applicable @JuliaKirschenheuter?

@JuliaKirschenheuter JuliaKirschenheuter added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Apr 27, 2023
@JuliaKirschenheuter
Copy link
Contributor Author

Is the developing label still applicable @JuliaKirschenheuter?

Thanks for remark

@skjnldsv skjnldsv mentioned this pull request May 3, 2023
@JuliaKirschenheuter JuliaKirschenheuter force-pushed the fix/36921-fix-table-for-users-app branch from 77e8332 to b847914 Compare May 5, 2023 17:47
This was referenced May 9, 2023
@ChristophWurst
Copy link
Member

COnflicts. CI stopped to free resources for mergeable PRs

@juliushaertl
Copy link
Member

I think removing the stickyness of the right action buttons is quite bad for a general user experience. Can you elaborate on the reasons why sticky is not working?

Also cc @jancborchardt for that

@JuliaKirschenheuter
Copy link
Contributor Author

I think removing the stickyness of the right action buttons is quite bad for a general user experience. Can you elaborate on the reasons why sticky is not working?

Sticky on master was already broken. Because z-indexes were wrong implemented, please look in:
#37860 (comment)

Current style was a bit incorrect:
.userActions had z-index: 10
.popovermenu had z-index: 110
.userActions is a parent of .popovermenu
A child cannot have z-index higher than parent
For position: relative we could just remove z-index: 10. But with position: sticky it works different... position: sticky always creates a new stacking context. It is possible neither to remove z-index from sticky parent, nor to make child with higher z-index...
If I'm not wrong, the only way to keep sticky here is to use another container for NcActions here. For example, #app-content. But I am really not sure, it positions element correct with sticky...

Current master:

Peek 2023-05-10 15-14

@ShGKme
Copy link
Contributor

ShGKme commented May 10, 2023

Proposals regarding sticky + z-index issue:

Render NcActions outside the table row

  • Props:
    • I'd expect it to be easy to implement
  • Cons:
    • Requires rewriting of some integration tests because row buttons will be outside of the row

Generate progressive z-index - set each next row less z-index value

  • Props:
    • Keep the current structure
  • Cons:
    • More complex implementation
    • Not sure about performance and large tables

@AndyScherzinger
Copy link
Member

@juliushaertl any suggestion how to improve this? Also @jancborchardt
The table is needed for a11y reasons, yet for the moment stickiness got removed but was already broken before. So I would then rather like to get the table issue fixed via this PR and have a follow-up fixing the stickiness which would not necessarily have to be done by the people working on a11y improvements.

@jancborchardt
Copy link
Member

So I would then rather like to get the table issue fixed via this PR and have a follow-up fixing the stickiness which would not necessarily have to be done by the people working on a11y improvements.

@AndyScherzinger Sounds reasonable – I missed that the stickyness was already broken on master as mentioned by @JuliaKirschenheuter.

Let’s keep it separate, get the table fixes in, and fix the stickyness separately. (Just for reference, we got reports for it not being discoverable which is why we added it. :)

@jancborchardt
Copy link
Member

So then 👍 from my point, but please open a follow-up issue so we can track the stickyness problem. :)

@JuliaKirschenheuter
Copy link
Contributor Author

Ok, sticky issue has been already existed. I would suggest to move forwards without sticky for now and i will open an issue to create action buttons with right sticky implementation.

Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

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

Looks good design-wise! :)

Signed-off-by: julia.kirschenheuter <julia.kirschenheuter@nextcloud.com>
@JuliaKirschenheuter JuliaKirschenheuter force-pushed the fix/36921-fix-table-for-users-app branch from a108936 to cb852ef Compare May 11, 2023 14:40
Copy link
Contributor

@ShGKme ShGKme left a comment

Choose a reason for hiding this comment

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

Loos good by code, tested on light and dark theme, different sizes, desktop and mobile.

Found a problem with the table header sticking, but it also appears on the master.

1

@JuliaKirschenheuter JuliaKirschenheuter added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels May 12, 2023
@JuliaKirschenheuter JuliaKirschenheuter merged commit 0d3df2c into master May 12, 2023
@JuliaKirschenheuter JuliaKirschenheuter deleted the fix/36921-fix-table-for-users-app branch May 12, 2023 07:33
@ChristophWurst
Copy link
Member

ChristophWurst commented May 12, 2023

two regressions

  1. table doesn't align, td columns float to the left
  2. actions background element doesn't match row background

Bildschirmfoto vom 2023-05-12 12-02-14

@ChristophWurst
Copy link
Member

^ @JuliaKirschenheuter

@ChristophWurst
Copy link
Member

The bug shows on our pre-production testing instance too. Filed #38245. @JuliaKirschenheuter please have a look

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BITV] 9.1.3.1e/8.1 - The user table represents a grid element and should be implemented as such. (1)
8 participants