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

Adding ability to fix table columns in place #7019

Merged
merged 4 commits into from
Jul 17, 2024

Conversation

ezraodio1
Copy link
Contributor

@ezraodio1 ezraodio1 commented Jun 14, 2024

What type of PR is this?

  • Refactor
  • Feature
  • Bug Fix
  • New Query Runner (Data Source)
  • New Alert Destination
  • Other

Description

This change involved adding an extra option to the GridSettings editor, adding the "fixed" option to columns, and adding styling for the fixed columns. In order to change the number of fixed columns, which will default to 0, one has to go to Edit visualization -> Grid -> Choose number of columns to fix -> Save.

How is this tested?

  • Unit tests (pytest, jest)
  • E2E Tests (Cypress)
  • Manually
  • N/A

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

My.Movie.2.mp4

This change involved adding an extra option to the GridSettings editor,
adding the "fixed" option to columns, and adding styling for the fixed
columns. In order to change the number of fixed columns, which will
default to 0, one has to go to Edit visualization -> Grid -> Choose
number of columns to fix -> Save.
@ezraodio1
Copy link
Contributor Author

ezraodio1 commented Jun 14, 2024

The frontend e2e Cypress tests that are failing are due to an invisible row that's being added at the top of the table (see attached screenshot). This, for some reason, is due to the following line of code in Renderer.tsx:
scroll = {{x : 'max-content'}}

The error persists with scroll = {{x : true}} and scroll = {{x : 500}}

Per Ant Design documentation, "To fix some columns and scroll inside other columns, and you must set scroll.x meanwhile."

Screenshot 2024-06-14 at 4 15 17 PM

@ezraodio1
Copy link
Contributor Author

In order to fix this issue, I changed the table.js, and therefore the Cypress tests that were failing, to not count this invisible row.

Without this, Cypress was counting the
.ant-table-measure-row, which is an invisible row
at the top of the table created by
scroll = {{x : 'max-content'}} in Renderer.tsx.
This was causing the query/filters_spec.js and
dashboard/filters_spec.js Cypress tests to fail.
Copy link
Collaborator

@eradman eradman left a comment

Choose a reason for hiding this comment

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

Very nice. Tested manually, no issues found

@eradman eradman enabled auto-merge (squash) July 17, 2024 13:28
@eradman eradman merged commit ebb0e2c into getredash:master Jul 17, 2024
11 checks passed
@justinclift
Copy link
Member

@ezraodio1 @eradman We've received a credible report that this PR broke vertical scrolling behaviour when there's greater than some number (25?) results.

Any ideas? I'm not yet super clueful with the front end pieces of Redash, so can't be super constructive with this bit just yet.

@justinclift justinclift mentioned this pull request Aug 23, 2024
2 tasks
eradman added a commit to eradman/redash that referenced this pull request Aug 23, 2024
eradman added a commit to StarfishStorage/redash that referenced this pull request Aug 27, 2024
This reverts commit ebb0e2c.

This feature disabled vertical scroll in query results:
getredash#7127
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