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

remove unused style; apply 95vw max width to users table in Facility #13131

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

nucleogenesis
Copy link
Member

Summary

Addresses issue noted in recent bug hunt by @rtibbles regarding the users table's width (or lack thereof).

There's so much screen available - so why would we limit the big table of data to the same width we use for settings forms and such?

The changes here remove an unused wrapper div that duplicated widths enforced elsewhere.

Then using a deep style we target our own main-wrapper style to override the page width specifically for the Users table without affecting the other Facility pages.

Screencast_20250228_091120.webm

References

#13127 lists the issue & links to the relevant Notion

Reviewer guidance

  • Try users table on various screen sizes. Does it look okay?
  • Check all other pages in Facility and see that they don't have any regressions.

@github-actions github-actions bot added APP: Facility Re: Facility App (user/class management, facility settings, csv import/export, etc.) DEV: frontend labels Feb 28, 2025
Copy link
Member

@marcellamaki marcellamaki left a comment

Choose a reason for hiding this comment

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

I know Richard is going to review, but just popping in with a couple of thoughts.

I definitely get the intention here, and I think it's a good point that this container might be "artificially" constrained, and that for some scenarios having a wider view would make more sense. However, I am a bit wary of overriding these styles in this way (sometimes it is definitely the right approach, but here, my spidey senses are tingling a little bit) and also I don't really want to change the view without looping in the designers.

This doesn't need to be resolved urgently for 0.18 - again, these are quick fix bugs (or intended to be). Maybe the next step is just talking through this a little more, and then evaluating it's priority - Is there another approach that could work here? What have you already thought through as options? i.e. What determines the width of the KTable columns, and are there existing props to make those more flexible? Should there be. Or from a different perspective - do we want to plan for some more dynamic page widths in Kolibri certain scenarios? (basically, the approach here, but with just a little bit more of input/team decision making)

Maybe let's check in over slack to loop in the designers next week - I don't think addressing this needs to be top of your todo list today :)

@MisRob
Copy link
Member

MisRob commented Feb 28, 2025

What determines the width of the KTable columns, and are there existing props to make those more flexible? Should there be.

Each KTable column width is configurable via minWidth and width attributes of the headers. There's "Table with custom column widths" section in the docs with examples.

With this I am not suggesting we take this approach - as @marcellamaki says better to chat with relevant folks, perhaps we indeed want to have more space for larger tables. Just a reference if that shows to be helpful.

@nucleogenesis nucleogenesis requested a review from jtamiace March 10, 2025 19:16
@nucleogenesis
Copy link
Member Author

@marcellamaki I chatted w/ @rtibbles a bit about the reported issue and he confirmed that the issue he was reporting was the artificial constraint on the width, so that's what I addressed here. I initially was thinking about the guts of the table and stuff but after chatting w/ Richard it became apparent I was overthinking the reported issue 😅

IMO this is a pretty quick win because showing a data table using only ~33-50% of the available width is a poor use of the available space in-context. In any case, happy to update this as discussion proceeds.

@tomiwaoLE and/or @jtamiace could you give this a look and let us know what you think of this change?

@@ -495,4 +495,9 @@
overflow-x: auto;
}

/deep/ .main-wrapper {
Copy link
Member

Choose a reason for hiding this comment

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

I think my main concern here (and I think this was also what @marcellamaki was trying to express) is not that having a wider container is a bad thing, but using one of /deep/ or !important would be a code smell, so doing both together is a code... stink?

We have already changed our overall width of page containers for much of the learn experience, so it doesn't seem like we're bound to that decision from ~2018.

@jtamiace
Copy link
Member

I don't think I can make a recommendation on the best code snippet to use for defining the max width, but in general I do think that using more of the screen space is helpful for data tables like this and this is a good move.

The only case I can think of where 95vw might degrade the UX and make it a little more difficult to read is when someone is using a very wide screen and the text in each cell is distributed far apart from each other. Maybe a non-issue because I assume many of our users won't be managing users full width on 40+ inch screens? But I wonder if it makes sense to introduce an upper limit and if one of the KDS container or grid components can be customized to display these large data tables at a width that feels right.

@MisRob
Copy link
Member

MisRob commented Mar 17, 2025

Linking #13104

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
APP: Facility Re: Facility App (user/class management, facility settings, csv import/export, etc.) DEV: frontend
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Facility > Users - Persistent bottom scrollbar
5 participants