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

docs: More specs for ui.table functionality #198

Merged
merged 7 commits into from
Feb 16, 2024

Conversation

jnumainville
Copy link
Collaborator

@jnumainville jnumainville commented Jan 9, 2024

Here are how my initial attempts line up with the requests:
Display names (#105): column_display_names
event listener (Fixes #180): add_event_listener

requests from #77:
Show/hide quick filter by default: display_quick_filters
density: density
Ability to hide column headers: display_column_headers
Ability for the server to specify rows to select: selected
Allow multi/single selection: added multi_select arg to selection_mode
Allow row selection to be visible with checkboxes (vs just styled as highlighted, etc): selection_style
Allow columns to be kept hidden, even though they are subscribed (and so in the payload to be sent to the server on events): I believe this is already spec'd as always_fetch_columns

Copy link
Contributor

@dsmmcken dsmmcken left a comment

Choose a reason for hiding this comment

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

  1. I think anything that is a setting should be a prop to ui.table, not method. Especially things that exist on other components as props. Like density would be prop on list for example.
  2. display_X would be show_X in spectrum, we should be consistent.
  3. Probably should just have a bunch of on_* events, not add_listener

plugins/ui/DESIGN.md Outdated Show resolved Hide resolved
plugins/ui/DESIGN.md Outdated Show resolved Hide resolved
plugins/ui/DESIGN.md Outdated Show resolved Hide resolved
plugins/ui/DESIGN.md Outdated Show resolved Hide resolved
plugins/ui/DESIGN.md Outdated Show resolved Hide resolved
plugins/ui/DESIGN.md Outdated Show resolved Hide resolved
plugins/ui/DESIGN.md Outdated Show resolved Hide resolved
@jnumainville
Copy link
Collaborator Author

  1. I think anything that is a setting should be a prop to ui.table, not method. Especially things that exist on other components as props. Like density would be prop on list for example.

What defines a setting vs not a setting?

plugins/ui/DESIGN.md Outdated Show resolved Hide resolved
plugins/ui/DESIGN.md Outdated Show resolved Hide resolved
plugins/ui/DESIGN.md Outdated Show resolved Hide resolved
plugins/ui/DESIGN.md Outdated Show resolved Hide resolved
plugins/ui/DESIGN.md Outdated Show resolved Hide resolved
plugins/ui/DESIGN.md Outdated Show resolved Hide resolved
plugins/ui/DESIGN.md Outdated Show resolved Hide resolved
plugins/ui/DESIGN.md Outdated Show resolved Hide resolved
plugins/ui/DESIGN.md Outdated Show resolved Hide resolved
plugins/ui/DESIGN.md Outdated Show resolved Hide resolved
plugins/ui/DESIGN.md Outdated Show resolved Hide resolved
plugins/ui/DESIGN.md Outdated Show resolved Hide resolved
plugins/ui/DESIGN.md Outdated Show resolved Hide resolved
@jnumainville jnumainville self-assigned this Feb 8, 2024
@jnumainville
Copy link
Collaborator Author

@dsmmcken any further thoughts on props vs functions?
Bender and I were thinking that it should all be functions.
It's definitely a bit odd in how it behaves compared to other components, but it mirrors what is already done with table styling, which I think is more important.

@dsmmcken
Copy link
Contributor

dsmmcken commented Feb 8, 2024

mirrors what is already done with table styling?

What do you mean by that?

My opinion is:

  1. Anything that is a prop on spectrum components should be a prop. (ie Density is a spectrum prop, we shouldn't break that pattern).
  2. Anything that only accepts one parameter should just be a prop, especially if it's a Boolean. The only reason we have so many methods here is because things like data_bars needs a million args.

@jnumainville
Copy link
Collaborator Author

jnumainville commented Feb 8, 2024

What I have in mind is cases like format_columns - it's a function that returns a new table.
What I'm getting at is the idea of immutable fluent tables.

Allowing props in that way could make some things trickier. Because we have the immutable fluent interface for some methods, we would need to do it in a way that would mesh nicely. Otherwise making slightly different versions of tables could be more complicated than it would need to be.
For example, say I was working with the density and can_search props. If I want two tables that only differ by those values, I have to specify that when creating the two tables, then apply the same immutable fluent transformations (data_bars, etc.) to both tables, rather than applying those transformations, then the density function for one and the can_search method for the other. It's not too complicated to abstract the former, but still more work than it should be.

So what I personally would be on board with is making the _with_prop (and perhaps other prop methods) public. They could also be passed on initial ui.table creation I suppose, but this way it's flexible and cohesive with the immutable fluent interface. Maybe we could have with_props that just takes a dictionary of simple props. Whether we have both functions and a with_props method for props such as density is something I have no opinion on, as at that point it's mostly a difference of documentation.

@jnumainville
Copy link
Collaborator Author

Decision is single variable functions should be props instead
if there is desire, individual props could be added as functions easily in the future

plugins/ui/DESIGN.md Outdated Show resolved Hide resolved
plugins/ui/DESIGN.md Outdated Show resolved Hide resolved
@jnumainville
Copy link
Collaborator Author

jnumainville commented Feb 13, 2024

I've moved every single argument method as well as others that decomposed easily (specifically selected) into props

Honestly, I think all of the methods could be refactored into props. data_bars is one of the more complicated ones but I don't think it would be that much more complicated as a dictionary of column name to attributes that's passed in instead. That's what many of these more complicated methods are going to become under the hood anyways.

Others like format for example could easily be a dictionary prop. And others like context_menu could just be two different props. But, I won't do these unless we want to go full on props and step away from the immutable fluent model completely or convert a few others to props, either way.

@dsmmcken
Copy link
Contributor

Looking good.

I know can_search was an existing layout hint, but we should lean into the feature disabling/enabling thing for ui table. We've had asks like this before.

My proposal. Remove can_search as a prop and backlog it. Add a backlog ticket to flesh this out in the future as its own method with a million options.

Something like:

ui.table(...).permissions(
  search=false,
  filter=false,
  rollup=false,
  ...
  sidebar_menu 
  sort
  copy
  download
  re-order
  etc.
)

Not sure what exactly to call it. Permissions? features? disable?

@jnumainville
Copy link
Collaborator Author

I think that's a good idea
Maybe allowed if that's not too broad
I like permissions although not with bool options, maybe with sets so you don't have to type all the bools out? although it has it's own downsides

ui.table(...).permissions(allowed={'search'}, not_allowed={'filter', 'rollup'})

maybe with enums so it more closely resembles other access control
Although I can save further syntax discussion for the ticket

Copy link
Contributor

@dsmmcken dsmmcken left a comment

Choose a reason for hiding this comment

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

No further comments.

@jnumainville jnumainville merged commit 8d4255c into deephaven:main Feb 16, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Spec for ui.table on_change events
3 participants