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.

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.

3 participants