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

Fix timeseries chrome overflow #42761

Merged
merged 8 commits into from
May 30, 2024
Merged

Fix timeseries chrome overflow #42761

merged 8 commits into from
May 30, 2024

Conversation

romeovs
Copy link
Contributor

@romeovs romeovs commented May 16, 2024

Closes #42760

Description

Describe the overall approach and the problem being solved.

How to verify

Describe the steps to verify that the changes are working as expected.

  1. Browse data -> Accounts
  2. Add an aggregation on a time column and pick a bucket
  3. Open the View ... by ... dropdown underneath the visualization

Demo

Screenshot 2024-05-16 at 14 21 57

Copy link

github-actions bot commented May 16, 2024

Codenotify: Notifying subscribers in CODENOTIFY files for diff 80687b8...11cee1a.

Notify File(s)
@kdoh frontend/src/metabase/ui/components/overlays/Menu/Menu.styled.tsx

Copy link

replay-io bot commented May 16, 2024

Status Complete ↗︎
Commit 11cee1a
Results
⚠️ 1 Flaky
2567 Passed

@romeovs romeovs added the no-backport Do not backport this PR to any branch label May 16, 2024
@romeovs romeovs changed the title [WIP]: Fix timeseries chrome overflow Fix timeseries chrome overflow May 16, 2024
@romeovs romeovs requested review from a team and mazameli May 16, 2024 15:15
Copy link
Contributor

@mazameli mazameli left a comment

Choose a reason for hiding this comment

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

Minor, but ideally the More… would be text-brand in color to distinguish it from the selectable items, and to match what we do in the summarize sidebar and notebook group-by popovers.

@@ -87,11 +87,23 @@ describe("TimeseriesBucketPicker", () => {
expect(getNextBucketName()).toBe("Year");
});

it("should allow to show more binning options", async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please also add tests for the selected item - when it's within the initial expanded list or not

Copy link
Contributor

@ranquild ranquild left a comment

Choose a reason for hiding this comment

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

Looks good, minor comments

@romeovs romeovs requested a review from ranquild May 21, 2024 11:54
@ranquild
Copy link
Contributor

@mazameli could you take a look?

@romeovs romeovs requested a review from mazameli May 30, 2024 11:15
@romeovs romeovs merged commit 378f0de into master May 30, 2024
111 checks passed
@romeovs romeovs deleted the fix-timeseries-chrome-overflow branch May 30, 2024 19:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-backport Do not backport this PR to any branch .Team/QueryingComponents
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The new time granularity option is too intense and overflows
3 participants