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

Expand cell height if there is only one row #537

Merged
merged 18 commits into from
Feb 11, 2025

Conversation

natalyjazzviolin
Copy link
Contributor

@natalyjazzviolin natalyjazzviolin commented Feb 3, 2025

Helps with https://github.com/ClickHouse/control-plane/issues/12806

Why

Currently if there is one row of data displayed, and it's something like a SHOW CREATE TABLE query, it is cut off by default:
Screenshot 2025-02-03 at 5 44 29 PM

What

This adds and exposes a rowAutoHeight prop to the Grid component. It should be set for the case when there is only one row. When set, it allows the cell height to expand to accommodate the content:

Screen.Recording.2025-02-07.at.8.36.34.AM.mov

Reproduce

  • Check out this PR
  • Run npm run build
  • Set the click-ui version in sql-console/web/package.json to "@clickhouse/click-ui": "../../../click-ui",
  • Run rm -rf node_modules && yarn cache clean && yarn && yarn build:packages
    Create the following table and show the create query:
CREATE TABLE random_user_events (
    user_id UInt32,
    event_time DateTime,
    event_type Enum8('click' = 1, 'view' = 2, 'purchase' = 3),
    item_id String,
    price Decimal(10,2),
    quantity UInt16
) ENGINE = MergeTree()
ORDER BY (user_id, event_time)
PARTITION BY toYYYYMM(event_time)
SETTINGS index_granularity = 8192;

show create table random_user_events;

Copy link

vercel bot commented Feb 3, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
click-ui ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 10, 2025 10:38pm

Copy link
Collaborator

@vineethasok vineethasok left a comment

Choose a reason for hiding this comment

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

Need to make. sure the height of the index is also updated using this logic
I would recommend adding a prop with rows with autoheight or a prop to say the height can be auto for all the rows or none of them instead of only first rows
Would love your thoughts @gjones @fneves @serdec

@natalyjazzviolin
Copy link
Contributor Author

@vineethasok I followed your suggestion and removed isOnlyRow prop and instead created a rowAutoHeight prop that the Grid component accepts and hands down to the StyledCell where the appropriate height styles are set.

I was hoping to set a matching height on the index column using CSS, but unfortunately the cells are in position: absolute, so that doesn't work. Currently my idea is to create a ref for the cell's height and set the index cell's height based on that. Would that make sense, or is there another approach to consider? Thank you!
cc: @fneves @hoorayimhelping

@natalyjazzviolin
Copy link
Contributor Author

Updated the initial description with the new implementation per the requested changes.

Copy link
Collaborator

@hoorayimhelping hoorayimhelping left a comment

Choose a reason for hiding this comment

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

Have a few nits and comments

Copy link
Collaborator

@hoorayimhelping hoorayimhelping left a comment

Choose a reason for hiding this comment

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

Let's light this candle and see what happens. I want you to keep an eye on this in Slack and escalations for the next few months: listen for reports of weird issues with grids and tables or bad performance. (Not saying this PR will cause those, but since this is a library that affects multiple areas of the application, we have a responsibility to make sure our changes don't cause regressions).

@natalyjazzviolin natalyjazzviolin merged commit 349a672 into main Feb 11, 2025
6 checks passed
@natalyjazzviolin natalyjazzviolin deleted the nataly/expand-cell-height branch February 11, 2025 19:42
hoorayimhelping added a commit that referenced this pull request Feb 20, 2025
@hoorayimhelping hoorayimhelping mentioned this pull request Feb 20, 2025
hoorayimhelping added a commit that referenced this pull request Feb 20, 2025
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.

4 participants