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.

Sign up for GitHub

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

Jump to bottom

chore: cleanup LemonTable API #18862

Merged
merged 7 commits into from
Dec 6, 2023
Merged

chore: cleanup LemonTable API #18862

merged 7 commits into from
Dec 6, 2023

Conversation

daibhin
Copy link
Contributor

@daibhin daibhin commented Nov 23, 2023 •
edited
Loading

Problem

We had the display option on the LemonTable that was only used in one place in the entire app.

Changes

  • Simplify borderless-rows, stealth and embedded style options to only have two
  • Stealth
    • removes the background including the header
    • removes all inner borders
  • Embedded
    • removes outer border and border radius
    • Does not impact background color

How did you test this code?

Visually. All tables I was testing look the same or better

billing products

Screenshot 2023-11-23 at 16 16 07

detailed results

Screenshot 2023-11-23 at 16 16 00

events in notebooks

Screenshot 2023-11-23 at 16 15 53

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 23, 2023 •
edited
Loading

Size Change: 0 B

Total Size: 1.83 MB

ℹ️ View Unchanged
Filename Size
frontend/dist/toolbar.js 1.83 MB

compressed-size-action

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

1 snapshot changes in total. 0 added, 1 modified, 0 deleted:

  • chromium: 0 added, 1 modified, 0 deleted (diff for shard 1)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@Twixes Twixes self-requested a review November 24, 2023 10:32
@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

2 snapshot changes in total. 0 added, 2 modified, 0 deleted:

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

1 snapshot changes in total. 0 added, 1 modified, 0 deleted:

  • chromium: 0 added, 1 modified, 0 deleted (diff for shard 1)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@daibhin daibhin requested a review from a team November 27, 2023 15:17
Twixes
Twixes reviewed Nov 27, 2023
View reviewed changes
frontend/__snapshots__/scenes-app-insights--funnel-left-to-right-breakdown--light--webkit.png Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Let's make column group headers consistent with column headers.
BW this header text color seem very low opacity at 40%, kind of disabled. Can we use at least 50% of opacity? That's the standard for "very muted content" elsewhere in the UI

Twixes
Twixes approved these changes Nov 27, 2023
View reviewed changes
Copy link
Member

@Twixes Twixes left a comment

Choose a reason for hiding this comment

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

Added some extra snapshots to get a better look. ;)
Looks good, though one ask from me above on the contrast side.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

1 snapshot changes in total. 0 added, 1 modified, 0 deleted:

  • chromium: 0 added, 1 modified, 0 deleted (diff for shard 2)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

1 snapshot changes in total. 0 added, 1 modified, 0 deleted:

  • chromium: 0 added, 1 modified, 0 deleted (diff for shard 2)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

102 snapshot changes in total. 0 added, 102 modified, 0 deleted:

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

2 snapshot changes in total. 0 added, 2 modified, 0 deleted:

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@daibhin daibhin mentioned this pull request Nov 29, 2023
Merged
@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

252 snapshot changes in total. 0 added, 252 modified, 0 deleted:

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

1 snapshot changes in total. 0 added, 1 modified, 0 deleted:

  • chromium: 0 added, 1 modified, 0 deleted (diff for shard 1)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

2 snapshot changes in total. 0 added, 2 modified, 0 deleted:

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

2 snapshot changes in total. 0 added, 2 modified, 0 deleted:

  • chromium: 0 added, 2 modified, 0 deleted (diff for shard 2)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

1 snapshot changes in total. 0 added, 1 modified, 0 deleted:

  • chromium: 0 added, 1 modified, 0 deleted (diff for shard 2)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@daibhin daibhin force-pushed the dn-fix/table-stickies branch from 483449a to 2459398 Compare December 5, 2023 11:00
@daibhin daibhin force-pushed the dn-fix/table-stickies branch from 3d5afe1 to fccb243 Compare December 5, 2023 11:52
@daibhin daibhin changed the title fix: sticky table columns chore: cleanup LemonTable API Dec 5, 2023
@daibhin daibhin requested review from Twixes and a team December 5, 2023 14:44
thmsobrmlr
thmsobrmlr approved these changes Dec 5, 2023
View reviewed changes
Copy link
Contributor

@thmsobrmlr thmsobrmlr left a comment

Choose a reason for hiding this comment

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

Makes sense to me.

frontend/src/lib/lemon-ui/LemonTable/LemonTable.tsx Outdated Show resolved Hide resolved
@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

1 snapshot changes in total. 0 added, 1 modified, 0 deleted:

  • chromium: 0 added, 1 modified, 0 deleted (diff for shard 1)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

1 snapshot changes in total. 0 added, 1 modified, 0 deleted:

  • chromium: 0 added, 1 modified, 0 deleted (diff for shard 1)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@daibhin daibhin merged commit 8dece79 into master Dec 6, 2023
78 checks passed
@daibhin daibhin deleted the dn-fix/table-stickies branch December 6, 2023 09:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thmsobrmlr thmsobrmlr thmsobrmlr approved these changes

@Twixes Twixes Awaiting requested review from Twixes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@daibhin @posthog-bot @thmsobrmlr @Twixes